slab: Allow for type introspection during allocation

The allocator will already reject giant sizes seen from negative size arguments, so this commit mainly services as an example for initial type-based filtering. The size argument is checked for negative values in signed arguments, saturating any if found instead of passing them on.

For example, now the size is checked:

Before:

				/* %rdi unchecked */
  1eb:   be c0 0c 00 00          mov    $0xcc0,%esi
  1f0:   e8 00 00 00 00          call   1f5 <do_SLAB_NEGATIVE+0x15>
                         1f1: R_X86_64_PLT32 __kmalloc_noprof-0x4

After:

  6d0:   48 63 c7                movslq %edi,%rax
  6d3:   85 ff                   test   %edi,%edi
  6d5:   be c0 0c 00 00          mov    $0xcc0,%esi
  6da:   48 c7 c2 ff ff ff ff    mov    $0xffffffffffffffff,%rdx
  6e1:   48 0f 49 d0             cmovns %rax,%rdx
  6e5:   48 89 d7                mov    %rdx,%rdi
  6e8:   e8 00 00 00 00          call   6ed <do_SLAB_NEGATIVE+0x1d>
                         6e9: R_X86_64_PLT32     __kmalloc_noprof-0x4

Signed-off-by Kees Cook <[email protected]>

---
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Hyeonggon Yoo <[email protected]>
Cc: [email protected]

---
  include/linux/slab.h | 19 ++++++++++++++++++-
  1 file changed, 18 insertions(+), 1 deletion(-)

include/linux/slab.h +8 -1

index d99afce36098..7353756cbec6 100644
@@ -684,7 +684,24 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
  	}
  	return __kmalloc_noprof(size, flags);
  }
-#define kmalloc(...)				alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+#define kmalloc_sized(...)			alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+
+#define __size_force_positive(x)				\
+	({							\
+		typeof(__force_integral_expr(x)) __forced_val =	\
+			__force_integral_expr(x);		\
+		__forced_val < 0 ? SIZE_MAX : __forced_val;	\
+	})

include/linux/slab.h +0 -0

  #define kmem_buckets_alloc(_b, _size, _flags)	\
  	alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b), _flags, NUMA_NO_NODE))

Quoted 59 lines · Kees Cook

The allocator will already reject giant sizes seen from negative size arguments, so this commit mainly services as an example for initial type-based filtering. The size argument is checked for negative values in signed arguments, saturating any if found instead of passing them on.

For example, now the size is checked:

Before:

				/* %rdi unchecked */
  1eb:   be c0 0c 00 00          mov    $0xcc0,%esi
  1f0:   e8 00 00 00 00          call   1f5 <do_SLAB_NEGATIVE+0x15>
                         1f1: R_X86_64_PLT32 __kmalloc_noprof-0x4

After:

  6d0:   48 63 c7                movslq %edi,%rax
  6d3:   85 ff                   test   %edi,%edi
  6d5:   be c0 0c 00 00          mov    $0xcc0,%esi
  6da:   48 c7 c2 ff ff ff ff    mov    $0xffffffffffffffff,%rdx
  6e1:   48 0f 49 d0             cmovns %rax,%rdx
  6e5:   48 89 d7                mov    %rdx,%rdi
  6e8:   e8 00 00 00 00          call   6ed <do_SLAB_NEGATIVE+0x1d>
                         6e9: R_X86_64_PLT32     __kmalloc_noprof-0x4

Signed-off-by Kees Cook <[email protected]>

---
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Hyeonggon Yoo <[email protected]>
Cc: [email protected]

---
  include/linux/slab.h | 19 ++++++++++++++++++-
  1 file changed, 18 insertions(+), 1 deletion(-)

include/linux/slab.h +12 -1

index d99afce36098..7353756cbec6 100644
@@ -684,7 +684,24 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
  	}
  	return __kmalloc_noprof(size, flags);
  }
-#define kmalloc(...)				alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+#define kmalloc_sized(...)			alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+
+#define __size_force_positive(x)				\
+	({							\
+		typeof(__force_integral_expr(x)) __forced_val =	\
+			__force_integral_expr(x);		\
+		__forced_val < 0 ? SIZE_MAX : __forced_val;	\
+	})
+
+#define kmalloc(p, gfp)		_Generic((p),    \
+	unsigned char:  kmalloc_sized(__force_integral_expr(p), gfp), \
+	unsigned short: kmalloc_sized(__force_integral_expr(p), gfp), \

include/linux/slab.h static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f · line 704

Quoted 6 lines

include/linux/slab.h +6 -0

+	unsigned int:   kmalloc_sized(__force_integral_expr(p), gfp), \
+	unsigned long:  kmalloc_sized(__force_integral_expr(p), gfp), \
+	signed char:    kmalloc_sized(__size_force_positive(p), gfp), \
+	signed short:   kmalloc_sized(__size_force_positive(p), gfp), \
+	signed int:     kmalloc_sized(__size_force_positive(p), gfp), \
+	signed long:    kmalloc_sized(__size_force_positive(p), gfp))

I like this idea and series very much, thank you!

What about bool?
What about long long?

Using a short Coccinelle script, it is possible to replace the classic kmalloc code patterns with the typed information:

@alloc@
type TYPE;
TYPE *P;
expression GFP;
identifier ALLOC =~ "k[mz]alloc";
@@

	P = ALLOC(

(quoted diff) +1 -1

-		\(sizeof(*P)\|sizeof(TYPE)\), GFP)
+		P, GFP)

Show this just for kmalloc/kzalloc usage in fs/pstore as an example.

Doing this for all allocator calls in the kernel touches much more:

  11941 files changed, 22459 insertions(+), 22345 deletions(-)

And obviously requires some more wrappers for kv*alloc, devm_*alloc, etc.

Signed-off-by Kees Cook <[email protected]>

---
Cc: Tony Luck <[email protected]>
Cc: "Guilherme G. Piccoli" <[email protected]>
Cc: [email protected]

---
  fs/pstore/blk.c      | 2 +-
  fs/pstore/platform.c | 2 +-
  fs/pstore/ram.c      | 3 +--
  fs/pstore/ram_core.c | 2 +-
  fs/pstore/zone.c     | 2 +-
  5 files changed, 5 insertions(+), 6 deletions(-)

fs/pstore/blk.c +0 -0

index de8cf5d75f34..7bb9cacb380f 100644

fs/pstore/blk.c static int __init __best_effort_init(void) · line 301

Quoted 11 lines · Kees Cook

fs/pstore/blk.c +1 -1

index de8cf5d75f34..7bb9cacb380f 100644
@@ -297,7 +297,7 @@ static int __init __best_effort_init(void)
  		return -EINVAL;
  	}
-	best_effort_dev = kzalloc(sizeof(*best_effort_dev), GFP_KERNEL);
+	best_effort_dev = kzalloc(best_effort_dev, GFP_KERNEL);
  	if (!best_effort_dev)
  		return -ENOMEM;

I expect raised eyebrows and typical vocalizations of amusement :D -
IOW: we should consider changing the name of the macro, although I like it as is :)

other:
you repeat the pointer name twice, and code is magic anyway, so perhaps:

	kzalloc_me(best_effort_dev, GFP_KERNEL);

and another variant to cover declaration-with-init.

Hi,

This is an RFC for some changes I'd like to make to the kernel's allocators (starting with slab) that allow for type introspection, which has been a long-time gap in potential analysis capabilities available at compile-time. The changes here are just a "first step" example that updates kmalloc() and kzalloc() to show what I'm thinking we can do, and shows an example conversion within the fs/pstore tree.

Repeating patch 3's commit log here:

There is currently no way for the slab to know what type is being allocated, and this hampers the development of any logic that would need this information including basic type checking, alignment need analysis, etc.

Allow the size argument to optionally be a variable, from which the type (and there by the size, alignment, or any other features) can be determined at compile-time. This allows for the incremental replacement of the classic code pattern:

obj = kmalloc(sizeof(*obj), gfp);

into:

obj = kmalloc(obj, gfp);

As an additional build-time safety feature, the return value of kmalloc() also becomes typed so that the assignment and first argument cannot drift, doing away with the other, more fragile, classic code pattern:

obj = kmalloc(sizeof(struct the_object), gfp);

into:

obj = kmalloc(obj, gfp);

Quoted 31 lines · Kees Cook

Hi,

This is an RFC for some changes I'd like to make to the kernel's allocators (starting with slab) that allow for type introspection, which has been a long-time gap in potential analysis capabilities available at compile-time. The changes here are just a "first step" example that updates kmalloc() and kzalloc() to show what I'm thinking we can do, and shows an example conversion within the fs/pstore tree.

Repeating patch 3's commit log here:

There is currently no way for the slab to know what type is being allocated, and this hampers the development of any logic that would need this information including basic type checking, alignment need analysis, etc.

Allow the size argument to optionally be a variable, from which the type (and there by the size, alignment, or any other features) can be determined at compile-time. This allows for the incremental replacement of the classic code pattern:

obj = kmalloc(sizeof(*obj), gfp);

into:

obj = kmalloc(obj, gfp);

As an additional build-time safety feature, the return value of kmalloc() also becomes typed so that the assignment and first argument cannot drift, doing away with the other, more fragile, classic code pattern:

Quoted 5 lines

obj = kmalloc(sizeof(struct the_object), gfp);

into:

obj = kmalloc(obj, gfp);

I like the idea, however it's not as simple and straightforward because it's common for structures to have a variable part (usually at the end) and also allocate more than one structure at once.

There are many allocations which look like

	kmalloc(sizeof(my_struct) * 2 + SOME_MAGIC_LENGTH, GFP_...)

or something like this, which you can't easily convert to your scheme.

The only option I see is to introduce the new set of functions/macros, something like kmalloc_obj() or kmalloc_struct(). Or maybe tmalloc()? (t for typed)

           obj = kmalloc(obj, gfp);

Quoted 2 lines · Kees Cook

           obj = kmalloc(obj, gfp);

Could we avoid repeating "obj" in this pattern?

F.e.

	KMALLOC(obj, gfp);

Quoted 7 lines · Christoph Lameter (Ampere)

Quoted 2 lines · Kees Cook

           obj = kmalloc(obj, gfp);

Could we avoid repeating "obj" in this pattern?

Quoted 3 lines

F.e.

      KMALLOC(obj, gfp);

This appears to be the common feedback, which is good! :) And we can still have it return "obj" as well, so it could still be used in "return" statements, etc. I will work up a new RFC...

Quoted 9 lines · Kees Cook

Quoted 7 lines · Christoph Lameter (Ampere)

Quoted 2 lines · Kees Cook

           obj = kmalloc(obj, gfp);

Could we avoid repeating "obj" in this pattern?

Quoted 7 lines

Quoted 3 lines

F.e.

      KMALLOC(obj, gfp);

This appears to be the common feedback, which is good! :) And we can still have it return "obj" as well, so it could still be used in "return" statements, etc. I will work up a new RFC...

More macros like this only obfuscate the code further. The name would become something that makes it really clear there's an assignment.

assign_kmalloc(obj, gfp)

There may be better options. Also ALLCAPS could be avoided here, as we have done with other language-like features (vs. pure constants).

Quoted 18 lines · Marco Elver

Quoted 9 lines · Kees Cook

Quoted 7 lines · Christoph Lameter (Ampere)

Quoted 2 lines · Kees Cook

            obj = kmalloc(obj, gfp);

Could we avoid repeating "obj" in this pattern?

Quoted 7 lines

Quoted 3 lines

F.e.

       KMALLOC(obj, gfp);

This appears to be the common feedback, which is good! :) And we can still have it return "obj" as well, so it could still be used in "return" statements, etc. I will work up a new RFC...

Quoted 7 lines

More macros like this only obfuscate the code further. The name would become something that makes it really clear there's an assignment.

assign_kmalloc(obj, gfp)

There may be better options. Also ALLCAPS could be avoided here, as we have done with other language-like features (vs. pure constants).

So, in looking a code patterns, it seems what we really want more than returning the object that was allocated is actually returning the size of the allocation size requested. i.e.:

	info->size = struct_size(ptr, flex_member, count);
	info->obj = kmalloc(info->size, gfp);

would become:

	info->size = kmalloc(info->obj, flex_member, count, gfp);

-Kees