All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] assoc_array: Avoid open coded arithmetic in allocator arguments
@ 2021-09-19 11:09 Len Baker
  2021-09-21 18:30 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 2+ messages in thread
From: Len Baker @ 2021-09-19 11:09 UTC (permalink / raw)
  To: Andrew Morton, Gustavo A. R. Silva, Kees Cook
  Cc: Len Baker, Miguel Ojeda, Nick Desaulniers, Nathan Chancellor,
	linux-hardening, linux-kernel

As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

So, use the struct_size() helper to do the arithmetic instead of the
argument "size + count * size" in the kmalloc() and kzalloc() functions.

Also, take the opportunity to refactor the memcpy() calls to use the
struct_size() and flex_array_size() helpers.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Signed-off-by: Len Baker <len.baker@gmx.com>
---
 lib/assoc_array.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index 04c98799c3ba..079c72e26493 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -741,8 +741,7 @@ static bool assoc_array_insert_into_terminal_node(struct assoc_array_edit *edit,
 	keylen = round_up(diff, ASSOC_ARRAY_KEY_CHUNK_SIZE);
 	keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;

-	new_s0 = kzalloc(sizeof(struct assoc_array_shortcut) +
-			 keylen * sizeof(unsigned long), GFP_KERNEL);
+	new_s0 = kzalloc(struct_size(new_s0, index_key, keylen), GFP_KERNEL);
 	if (!new_s0)
 		return false;
 	edit->new_meta[2] = assoc_array_shortcut_to_ptr(new_s0);
@@ -849,8 +848,8 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
 		keylen = round_up(diff, ASSOC_ARRAY_KEY_CHUNK_SIZE);
 		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;

-		new_s0 = kzalloc(sizeof(struct assoc_array_shortcut) +
-				 keylen * sizeof(unsigned long), GFP_KERNEL);
+		new_s0 = kzalloc(struct_size(new_s0, index_key, keylen),
+				 GFP_KERNEL);
 		if (!new_s0)
 			return false;
 		edit->new_meta[1] = assoc_array_shortcut_to_ptr(new_s0);
@@ -864,7 +863,7 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
 		new_n0->parent_slot = 0;

 		memcpy(new_s0->index_key, shortcut->index_key,
-		       keylen * sizeof(unsigned long));
+		       flex_array_size(new_s0, index_key, keylen));

 		blank = ULONG_MAX << (diff & ASSOC_ARRAY_KEY_CHUNK_MASK);
 		pr_devel("blank off [%zu] %d: %lx\n", keylen - 1, diff, blank);
@@ -899,8 +898,8 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
 		keylen = round_up(shortcut->skip_to_level, ASSOC_ARRAY_KEY_CHUNK_SIZE);
 		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;

-		new_s1 = kzalloc(sizeof(struct assoc_array_shortcut) +
-				 keylen * sizeof(unsigned long), GFP_KERNEL);
+		new_s1 = kzalloc(struct_size(new_s1, index_key, keylen),
+				 GFP_KERNEL);
 		if (!new_s1)
 			return false;
 		edit->new_meta[2] = assoc_array_shortcut_to_ptr(new_s1);
@@ -913,7 +912,7 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
 		new_n0->slots[sc_slot] = assoc_array_shortcut_to_ptr(new_s1);

 		memcpy(new_s1->index_key, shortcut->index_key,
-		       keylen * sizeof(unsigned long));
+		       flex_array_size(new_s1, index_key, keylen));

 		edit->set[1].ptr = &side->back_pointer;
 		edit->set[1].to = assoc_array_shortcut_to_ptr(new_s1);
@@ -1490,13 +1489,12 @@ int assoc_array_gc(struct assoc_array *array,
 		shortcut = assoc_array_ptr_to_shortcut(cursor);
 		keylen = round_up(shortcut->skip_to_level, ASSOC_ARRAY_KEY_CHUNK_SIZE);
 		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;
-		new_s = kmalloc(sizeof(struct assoc_array_shortcut) +
-				keylen * sizeof(unsigned long), GFP_KERNEL);
+		new_s = kmalloc(struct_size(new_s, index_key, keylen),
+				GFP_KERNEL);
 		if (!new_s)
 			goto enomem;
 		pr_devel("dup shortcut %p -> %p\n", shortcut, new_s);
-		memcpy(new_s, shortcut, (sizeof(struct assoc_array_shortcut) +
-					 keylen * sizeof(unsigned long)));
+		memcpy(new_s, shortcut, struct_size(new_s, index_key, keylen));
 		new_s->back_pointer = new_parent;
 		new_s->parent_slot = shortcut->parent_slot;
 		*new_ptr_pp = new_parent = assoc_array_shortcut_to_ptr(new_s);
--
2.25.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] assoc_array: Avoid open coded arithmetic in allocator arguments
  2021-09-19 11:09 [PATCH] assoc_array: Avoid open coded arithmetic in allocator arguments Len Baker
@ 2021-09-21 18:30 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 2+ messages in thread
From: Gustavo A. R. Silva @ 2021-09-21 18:30 UTC (permalink / raw)
  To: Len Baker, Andrew Morton, Gustavo A. R. Silva, Kees Cook
  Cc: Miguel Ojeda, Nick Desaulniers, Nathan Chancellor,
	linux-hardening, linux-kernel



On 9/19/21 06:09, Len Baker wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> So, use the struct_size() helper to do the arithmetic instead of the
> argument "size + count * size" in the kmalloc() and kzalloc() functions.
> 
> Also, take the opportunity to refactor the memcpy() calls to use the
> struct_size() and flex_array_size() helpers.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> 
> Signed-off-by: Len Baker <len.baker@gmx.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

I'll add this to my -next tree.

Thanks
--
Gustavo

> ---
>  lib/assoc_array.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> index 04c98799c3ba..079c72e26493 100644
> --- a/lib/assoc_array.c
> +++ b/lib/assoc_array.c
> @@ -741,8 +741,7 @@ static bool assoc_array_insert_into_terminal_node(struct assoc_array_edit *edit,
>  	keylen = round_up(diff, ASSOC_ARRAY_KEY_CHUNK_SIZE);
>  	keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;
> 
> -	new_s0 = kzalloc(sizeof(struct assoc_array_shortcut) +
> -			 keylen * sizeof(unsigned long), GFP_KERNEL);
> +	new_s0 = kzalloc(struct_size(new_s0, index_key, keylen), GFP_KERNEL);
>  	if (!new_s0)
>  		return false;
>  	edit->new_meta[2] = assoc_array_shortcut_to_ptr(new_s0);
> @@ -849,8 +848,8 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
>  		keylen = round_up(diff, ASSOC_ARRAY_KEY_CHUNK_SIZE);
>  		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;
> 
> -		new_s0 = kzalloc(sizeof(struct assoc_array_shortcut) +
> -				 keylen * sizeof(unsigned long), GFP_KERNEL);
> +		new_s0 = kzalloc(struct_size(new_s0, index_key, keylen),
> +				 GFP_KERNEL);
>  		if (!new_s0)
>  			return false;
>  		edit->new_meta[1] = assoc_array_shortcut_to_ptr(new_s0);
> @@ -864,7 +863,7 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
>  		new_n0->parent_slot = 0;
> 
>  		memcpy(new_s0->index_key, shortcut->index_key,
> -		       keylen * sizeof(unsigned long));
> +		       flex_array_size(new_s0, index_key, keylen));
> 
>  		blank = ULONG_MAX << (diff & ASSOC_ARRAY_KEY_CHUNK_MASK);
>  		pr_devel("blank off [%zu] %d: %lx\n", keylen - 1, diff, blank);
> @@ -899,8 +898,8 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
>  		keylen = round_up(shortcut->skip_to_level, ASSOC_ARRAY_KEY_CHUNK_SIZE);
>  		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;
> 
> -		new_s1 = kzalloc(sizeof(struct assoc_array_shortcut) +
> -				 keylen * sizeof(unsigned long), GFP_KERNEL);
> +		new_s1 = kzalloc(struct_size(new_s1, index_key, keylen),
> +				 GFP_KERNEL);
>  		if (!new_s1)
>  			return false;
>  		edit->new_meta[2] = assoc_array_shortcut_to_ptr(new_s1);
> @@ -913,7 +912,7 @@ static bool assoc_array_insert_mid_shortcut(struct assoc_array_edit *edit,
>  		new_n0->slots[sc_slot] = assoc_array_shortcut_to_ptr(new_s1);
> 
>  		memcpy(new_s1->index_key, shortcut->index_key,
> -		       keylen * sizeof(unsigned long));
> +		       flex_array_size(new_s1, index_key, keylen));
> 
>  		edit->set[1].ptr = &side->back_pointer;
>  		edit->set[1].to = assoc_array_shortcut_to_ptr(new_s1);
> @@ -1490,13 +1489,12 @@ int assoc_array_gc(struct assoc_array *array,
>  		shortcut = assoc_array_ptr_to_shortcut(cursor);
>  		keylen = round_up(shortcut->skip_to_level, ASSOC_ARRAY_KEY_CHUNK_SIZE);
>  		keylen >>= ASSOC_ARRAY_KEY_CHUNK_SHIFT;
> -		new_s = kmalloc(sizeof(struct assoc_array_shortcut) +
> -				keylen * sizeof(unsigned long), GFP_KERNEL);
> +		new_s = kmalloc(struct_size(new_s, index_key, keylen),
> +				GFP_KERNEL);
>  		if (!new_s)
>  			goto enomem;
>  		pr_devel("dup shortcut %p -> %p\n", shortcut, new_s);
> -		memcpy(new_s, shortcut, (sizeof(struct assoc_array_shortcut) +
> -					 keylen * sizeof(unsigned long)));
> +		memcpy(new_s, shortcut, struct_size(new_s, index_key, keylen));
>  		new_s->back_pointer = new_parent;
>  		new_s->parent_slot = shortcut->parent_slot;
>  		*new_ptr_pp = new_parent = assoc_array_shortcut_to_ptr(new_s);
> --
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-21 18:27 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19 11:09 [PATCH] assoc_array: Avoid open coded arithmetic in allocator arguments Len Baker
2021-09-21 18:30 ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.