[RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[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:
    
            obj = kmalloc(sizeof(struct the_object), gfp);
    
    into:
    
            obj = kmalloc(obj, gfp);
    
    And any accidental variable drift will not be masked by the traditional
    default "void *" return value:
    
            obj = kmalloc(something_else, gfp);
    
    error: assignment to 'struct the_object *' from incompatible pointer type 'struct foo *' [-Wincompatible-pointer-types]
       71 |     obj = kmalloc(something_else, gfp);
          |         ^
    
    This also opens the door for a proposed heap hardening feature that
    would randomize the starting offset of the allocated object within
    its power-of-2 bucket. Without being able to introspect the type for
    alignment needs, this can't be done safely (or cannot be done without
    significant memory usage overhead). For example, a 132 byte structure
    with an 8 byte alignment could be randomized into 15 locations within
    the 256 byte bucket: (256 - 132) / 8.


Thanks!

-Kees

Kees Cook (4):
  compiler_types: Add integral/pointer type helper macros
  slab: Detect negative size values and saturate
  slab: Allow for type introspection during allocation
  pstore: Replace classic kmalloc code pattern with typed argument

 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 +-
 include/linux/compiler_types.h | 23 +++++++++++++++++++++++
 include/linux/slab.h           | 32 +++++++++++++++++++++++++-------
 7 files changed, 53 insertions(+), 13 deletions(-)

-- 
2.34.1

[RFC][PATCH 1/4] compiler_types: Add integral/pointer type helper macros

[Kees Cook]

Many compiler builtins (e.g. _Generic, __builtin_choose_expr) perform
integral vs pointer argument type evaluation on outputs before evaluating
the input expression. This means that all outputs must be syntactically
valid for all inputs, even if the output would be otherwise filtering
the types. For example, this will fail to build:

 #define handle_int_or_ptr(x)					\
	_Generic((x),						\
		int: handle_int(x),				\
		int *: handle_ptr(x))
 ...
 handle_int_or_ptr(7);

error: passing argument 1 of 'handle_ptr' makes pointer from integer without a cast [-Wint-conversion]
  108 |     handle_int_or_ptr(x);
      |                       ^
      |                       |
      |                       int

To deal with this, provide helpers that force expressions into the
desired type, where the mismatched cases are syntactically value, but
will never actually happen:

 #define handle_int_or_ptr(x)					\
	_Generic((x),						\
		int: handle_int(__force_integral_expr(x)),	\
		int *: handle_ptr(__force_ptr_expr(x)))

Now handle_int() only ever sees an int, and handle_ptr() only ever sees
a pointer, regardless of the input type.

Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Marco Elver <elver@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Hao Luo <haoluo@google.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/linux/compiler_types.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index f14c275950b5..7754f3b6a91f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -450,6 +450,29 @@ struct ftrace_likely_data {
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
+/* Is the variable addressable? */
+#define __is_ptr_or_array(p)			\
+	(__builtin_classify_type(p) == __builtin_classify_type(NULL))
+
+/* Return an array decayed to a pointer. */
+#define __decay(p)				\
+	(&*__builtin_choose_expr(__is_ptr_or_array(p), p, NULL))
+
+/* Report if variable is a pointer type. */
+#define __is_ptr(p)		__same_type(p, __decay(p))
+
+/* Always produce an integral expression, with specific type/vale fallback. */
+#define ___force_integral_expr(x, type, val)	\
+	__builtin_choose_expr(!__is_ptr(x), (x), (type)val)
+#define __force_integral_expr(x)	\
+	___force_integral_expr(x, int, 0)
+
+/* Always produce a pointer expression, with specific type/value fallback. */
+#define ___force_ptr_expr(x, type, value)	\
+	__builtin_choose_expr(__is_ptr(x), (x), &(type){ value })
+#define __force_ptr_expr(x)	\
+	__builtin_choose_expr(__is_ptr(x), (x), NULL)
+
 /*
  * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
  *			       non-scalar types unchanged.
-- 
2.34.1

[RFC][PATCH 2/4] slab: Detect negative size values and saturate

[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 <kees@kernel.org>
---
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: linux-mm@kvack.org
---
 include/linux/slab.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index d99afce36098..7353756cbec6 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -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), \
+	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))
 
 #define kmem_buckets_alloc(_b, _size, _flags)	\
 	alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b), _flags, NUMA_NO_NODE))
-- 
2.34.1

[RFC][PATCH 3/4] slab: Allow for type introspection during allocation

[Kees Cook]

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);

And any accidental variable drift will not be masked by the traditional
default "void *" return value:

	obj = kmalloc(something_else, gfp);

error: assignment to 'struct the_object *' from incompatible pointer type 'struct foo *' [-Wincompatible-pointer-types]
   71 |     obj = kmalloc(something_else, gfp);
      |         ^

This also opens the door for a proposed heap hardening feature that
would randomize the starting offset of the allocated object within
its power-of-2 bucket. Without being able to introspect the type for
alignment needs, this can't be done safely (or cannot be done without
significant memory usage overhead). For example, a 132 byte structure
with an 8 byte alignment could be randomized into 15 locations within
the 256 byte bucket: (256 - 132) / 8.

Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: linux-mm@kvack.org
---
 include/linux/slab.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7353756cbec6..4a7350bbcfe7 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -685,6 +685,7 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
 	return __kmalloc_noprof(size, flags);
 }
 #define kmalloc_sized(...)			alloc_hooks(kmalloc_noprof(__VA_ARGS__))
+#define kmalloc_aligned(size, align, gfp)	alloc_hooks(kmalloc_noprof(size, gfp))
 
 #define __size_force_positive(x)				\
 	({							\
@@ -701,7 +702,10 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
 	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))
+	signed long:    kmalloc_sized(__size_force_positive(p), gfp), \
+	default:	(typeof(__force_ptr_expr(p)))kmalloc_aligned(  \
+				sizeof(*__force_ptr_expr(p)),	      \
+				__alignof__(*__force_ptr_expr(p)), gfp))
 
 #define kmem_buckets_alloc(_b, _size, _flags)	\
 	alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b), _flags, NUMA_NO_NODE))
@@ -816,14 +820,11 @@ static inline __alloc_size(1, 2) void *kmalloc_array_node_noprof(size_t n, size_
 
 /**
  * kzalloc - allocate memory. The memory is set to zero.
- * @size: how many bytes of memory are required.
+ * @p: either a pointer to an object to be allocated or bytes of memory are required.
  * @flags: the type of memory to allocate (see kmalloc).
  */
-static inline __alloc_size(1) void *kzalloc_noprof(size_t size, gfp_t flags)
-{
-	return kmalloc_noprof(size, flags | __GFP_ZERO);
-}
-#define kzalloc(...)				alloc_hooks(kzalloc_noprof(__VA_ARGS__))
+#define kzalloc(_p, _flags)			kmalloc(_p, (_flags)|__GFP_ZERO)
+#define kzalloc_noprof(_size, _flags)		kmalloc_noprof(_size, (_flags)|__GFP_ZERO)
 #define kzalloc_node(_size, _flags, _node)	kmalloc_node(_size, (_flags)|__GFP_ZERO, _node)
 
 void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node) __alloc_size(1);
-- 
2.34.1

[RFC][PATCH 4/4] pstore: Replace classic kmalloc code pattern with typed argument

[Kees Cook]

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(
-		\(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 <kees@kernel.org>
---
Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: linux-hardening@vger.kernel.org
---
 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(-)

diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index de8cf5d75f34..7bb9cacb380f 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -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;
 
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..4e527c3ea530 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -682,7 +682,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		struct pstore_record *record;
 		int rc;
 
-		record = kzalloc(sizeof(*record), GFP_KERNEL);
+		record = kzalloc(record, GFP_KERNEL);
 		if (!record) {
 			pr_err("out of memory creating record\n");
 			break;
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..a0665a98b135 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -228,8 +228,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 			 */
 			struct persistent_ram_zone *tmp_prz, *prz_next;
 
-			tmp_prz = kzalloc(sizeof(struct persistent_ram_zone),
-					  GFP_KERNEL);
+			tmp_prz = kzalloc(tmp_prz, GFP_KERNEL);
 			if (!tmp_prz)
 				return -ENOMEM;
 			prz = tmp_prz;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index f1848cdd6d34..01ddf1be6c3a 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -588,7 +588,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 	struct persistent_ram_zone *prz;
 	int ret = -ENOMEM;
 
-	prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
+	prz = kzalloc(prz, GFP_KERNEL);
 	if (!prz) {
 		pr_err("failed to allocate persistent ram zone\n");
 		goto err;
diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
index 694db616663f..8df890bb4db9 100644
--- a/fs/pstore/zone.c
+++ b/fs/pstore/zone.c
@@ -1165,7 +1165,7 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	zone = kzalloc(sizeof(struct pstore_zone), GFP_KERNEL);
+	zone = kzalloc(zone, GFP_KERNEL);
 	if (!zone)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.34.1

Re: [RFC][PATCH 2/4] slab: Detect negative size values and saturate

[Przemek Kitszel]

On 7/8/24 21:18, Kees Cook wrote:
> 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 <kees@kernel.org>
> ---
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Cc: linux-mm@kvack.org
> ---
>   include/linux/slab.h | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index d99afce36098..7353756cbec6 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -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), \
> +	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?
(by this commit one will get a rather easy to parse compile error, but
the next one will obscure it a bit)

Consider the following correct (albeit somewhat weird) code:
	/* header */
	char *state;

	/* .c impl, init part */
	bool needs_state = some_expr();
	state = kmalloc(needs_state, GFP_KERNEL);

	/* .c, other part */
	if (ZERO_OR_NULL_PTR(state))
		return _EARLY;
	*state = state_machine_action(*state);

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

Re: [RFC][PATCH 4/4] pstore: Replace classic kmalloc code pattern with typed argument

[Przemek Kitszel]

On 7/8/24 21:18, Kees Cook wrote:
> 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(
> -		\(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 <kees@kernel.org>
> ---
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> Cc: linux-hardening@vger.kernel.org
> ---
>   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(-)
> 
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index de8cf5d75f34..7bb9cacb380f 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -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.

Re: [RFC][PATCH 2/4] slab: Detect negative size values and saturate

[Kees Cook]

On Tue, Jul 09, 2024 at 08:57:55AM +0200, Przemek Kitszel wrote:
> On 7/8/24 21:18, Kees Cook wrote:
> > 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 <kees@kernel.org>
> > ---
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Roman Gushchin <roman.gushchin@linux.dev>
> > Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > Cc: linux-mm@kvack.org
> > ---
> >   include/linux/slab.h | 19 ++++++++++++++++++-
> >   1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index d99afce36098..7353756cbec6 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -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), \
> > +	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!

Thanks!

> What about bool?
> What about long long?

Ah yes, I will add these. LKP also found a weird one (a bitfield!) that
I'm fixing at the source:
https://lore.kernel.org/lkml/20240709154953.work.953-kees@kernel.org/

-- 
Kees Cook

Re: [RFC][PATCH 4/4] pstore: Replace classic kmalloc code pattern with typed argument

[Kees Cook]

On Tue, Jul 09, 2024 at 09:06:58AM +0200, Przemek Kitszel wrote:
> On 7/8/24 21:18, Kees Cook wrote:
> > diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> > index de8cf5d75f34..7bb9cacb380f 100644
> > --- a/fs/pstore/blk.c
> > +++ b/fs/pstore/blk.c
> > @@ -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 :)

Yeah, I prefer keeping the name too. I feel like adding yet more macros
around the allocators does not make anyone too happy. :)

> 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.

Switch the calling style is, however, where I think it'd be good
to change the name. I've had push-back in the past on changing away
from the "assignment style" to the "pass by reference" style, but I
would honestly prefer dropping the assignment: it is almost always
redundant. (I understood the push-back to be around the case of not
being able to easily use the "return kmalloc(...)" code pattern.)

It also makes it easier to deal with fixed array and flexible array
variants, as the argument count can be examined to determine if this is
a fixed-size struct or a flexible object:

	kzalloc_struct(ptr, GFP_KERNEL);

	kzalloc_struct(ptr, flex_member, count, GFP_KERNEL);
		-> uses struct_size() to get allocation size

I wonder if we can find a way to also handle the array case at compile
time:

	kzalloc_struct(array, count, GFP_KERNEL);

And if so, maybe the naming should be "kzalloc_me" like you suggest, or
maybe "kzalloc_obj"?

The resulting Coccinelle script gets a little more complex since we have
to rewrite the matched function, but it's not bad:

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

	P = ALLOC(\(sizeof(*P)\|sizeof(TYPE)\), GFP)

@script:python rename@
alloc_name << find.ALLOC;
ALLOC_OBJ;
@@

coccinelle.ALLOC_OBJ = cocci.make_ident(alloc_name + "_obj")

@convert@
type find.TYPE;
TYPE *find.P;
expression find.GFP;
identifier find.ALLOC;
identifier rename.ALLOC_OBJ;
@@

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

And the results in fs/pstore/ look like this:


diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index de8cf5d75f34..bc0e0a170604 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -297,7 +297,7 @@ static int __init __best_effort_init(void)
 		return -EINVAL;
 	}
 
-	best_effort_dev = kzalloc(sizeof(*best_effort_dev), GFP_KERNEL);
+	kzalloc_obj(best_effort_dev, GFP_KERNEL);
 	if (!best_effort_dev)
 		return -ENOMEM;
 
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..e0ba70543121 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -682,7 +682,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		struct pstore_record *record;
 		int rc;
 
-		record = kzalloc(sizeof(*record), GFP_KERNEL);
+		kzalloc_obj(record, GFP_KERNEL);
 		if (!record) {
 			pr_err("out of memory creating record\n");
 			break;
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..93064ba2c480 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -228,8 +228,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 			 */
 			struct persistent_ram_zone *tmp_prz, *prz_next;
 
-			tmp_prz = kzalloc(sizeof(struct persistent_ram_zone),
-					  GFP_KERNEL);
+			kzalloc_obj(tmp_prz, GFP_KERNEL);
 			if (!tmp_prz)
 				return -ENOMEM;
 			prz = tmp_prz;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index f1848cdd6d34..9561f4dfa853 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -588,7 +588,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 	struct persistent_ram_zone *prz;
 	int ret = -ENOMEM;
 
-	prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
+	kzalloc_obj(prz, GFP_KERNEL);
 	if (!prz) {
 		pr_err("failed to allocate persistent ram zone\n");
 		goto err;
diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
index 694db616663f..312796dc93f0 100644
--- a/fs/pstore/zone.c
+++ b/fs/pstore/zone.c
@@ -1165,7 +1165,7 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	zone = kzalloc(sizeof(struct pstore_zone), GFP_KERNEL);
+	kzalloc_obj(zone, GFP_KERNEL);
 	if (!zone)
 		return ERR_PTR(-ENOMEM);
 

-- 
Kees Cook

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Roman Gushchin]

On Mon, Jul 08, 2024 at 12:18:34PM -0700, Kees Cook wrote:
> 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);

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)

Thanks!

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Christoph Lameter (Ampere)]

On Mon, 8 Jul 2024, Kees Cook wrote:

>
>            obj = kmalloc(obj, gfp);

Could we avoid repeating "obj" in this pattern?

F.e.

 	KMALLOC(obj, gfp);

instead?

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Kees Cook]

On Tue, Jul 09, 2024 at 04:57:38PM +0000, Roman Gushchin wrote:
> On Mon, Jul 08, 2024 at 12:18:34PM -0700, Kees Cook wrote:
> > 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);
> 
> 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.

Right -- and with this we can leave those as-is initially (since a size
argument will still work).

> 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)

Yeah, in a neighboring thread I was talking about a kmalloc_obj that
would handle fixed-sized structs, flexible array structs, and arrays. I
need to prove out the array part, but the first two should be trivial to
implement.

-- 
Kees Cook

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Kees Cook]

On Tue, Jul 09, 2024 at 10:26:32AM -0700, Christoph Lameter (Ampere) wrote:
> On Mon, 8 Jul 2024, Kees Cook wrote:
> 
> > 
> >            obj = kmalloc(obj, gfp);
> 
> Could we avoid repeating "obj" in this pattern?
> 
> 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...

-- 
Kees Cook

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Marco Elver]

On Tue, 9 Jul 2024 at 22:28, Kees Cook <kees@kernel.org> wrote:
>
> On Tue, Jul 09, 2024 at 10:26:32AM -0700, Christoph Lameter (Ampere) wrote:
> > On Mon, 8 Jul 2024, Kees Cook wrote:
> >
> > >
> > >            obj = kmalloc(obj, gfp);
> >
> > Could we avoid repeating "obj" in this pattern?
> >
> > 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).

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Kees Cook]

On Tue, Jul 09, 2024 at 11:02:55PM +0200, Marco Elver wrote:
> On Tue, 9 Jul 2024 at 22:28, Kees Cook <kees@kernel.org> wrote:
> >
> > On Tue, Jul 09, 2024 at 10:26:32AM -0700, Christoph Lameter (Ampere) wrote:
> > > On Mon, 8 Jul 2024, Kees Cook wrote:
> > >
> > > >
> > > >            obj = kmalloc(obj, gfp);
> > >
> > > Could we avoid repeating "obj" in this pattern?
> > >
> > > 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).

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

-- 
Kees Cook

Re: [RFC][PATCH 0/4] slab: Allow for type introspection during allocation

[Przemek Kitszel]

On 7/10/24 01:28, Kees Cook wrote:
> On Tue, Jul 09, 2024 at 11:02:55PM +0200, Marco Elver wrote:
>> On Tue, 9 Jul 2024 at 22:28, Kees Cook <kees@kernel.org> wrote:
>>>
>>> On Tue, Jul 09, 2024 at 10:26:32AM -0700, Christoph Lameter (Ampere) wrote:
>>>> On Mon, 8 Jul 2024, Kees Cook wrote:
>>>>
>>>>>
>>>>>             obj = kmalloc(obj, gfp);
>>>>
>>>> Could we avoid repeating "obj" in this pattern?
>>>>
>>>> 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).
> 
> 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
> 

that will work out also for the (IMO) most common case of checking if
the allocation succeeded:
	if (!kmalloc(my_foo, flex_part, count, gfp))
		return -ENOMEM;