[v2] ext4: Annotate struct ext4_xattr_inode_array with __counted_by()

[PATCH v2] ext4: Annotate struct ext4_xattr_inode_array with __counted_by()

Posted by Thorsten Blum 1 year, 4 months ago

Add the __counted_by compiler attribute to the flexible array member
inodes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Remove the now obsolete comment on the count field.

Refactor ext4_expand_inode_array() by assigning count before copying any
data using memcpy(). Copy only the inodes array instead of the whole
struct because count has been set explicitly.

Use struct_size() and struct_size_t() instead of offsetof().

Change the data type of the local variable count to unsigned int to
match the struct's count data type.

Compile-tested only.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Adjust ext4_expand_inode_array() as suggested by Gustavo A. R. Silva
- Use struct_size() and struct_size_t() instead of offsetof()
- Link to v1: https://lore.kernel.org/linux-kernel/20240729110454.346918-3-thorsten.blum@toblux.com/
---
 fs/ext4/xattr.c | 20 +++++++++-----------
 fs/ext4/xattr.h |  4 ++--
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 46ce2f21fef9..b27543587103 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2879,11 +2879,10 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
 	if (*ea_inode_array == NULL) {
 		/*
 		 * Start with 15 inodes, so it fits into a power-of-two size.
-		 * If *ea_inode_array is NULL, this is essentially offsetof()
 		 */
 		(*ea_inode_array) =
-			kmalloc(offsetof(struct ext4_xattr_inode_array,
-					 inodes[EIA_MASK]),
+			kmalloc(struct_size_t(struct ext4_xattr_inode_array,
+					      inodes, EIA_MASK),
 				GFP_NOFS);
 		if (*ea_inode_array == NULL)
 			return -ENOMEM;
@@ -2891,17 +2890,16 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
 	} else if (((*ea_inode_array)->count & EIA_MASK) == EIA_MASK) {
 		/* expand the array once all 15 + n * 16 slots are full */
 		struct ext4_xattr_inode_array *new_array = NULL;
-		int count = (*ea_inode_array)->count;
+		unsigned int count = (*ea_inode_array)->count;
 
-		/* if new_array is NULL, this is essentially offsetof() */
-		new_array = kmalloc(
-				offsetof(struct ext4_xattr_inode_array,
-					 inodes[count + EIA_INCR]),
-				GFP_NOFS);
+		new_array = kmalloc(struct_size(*ea_inode_array, inodes,
+						count + EIA_INCR),
+				    GFP_NOFS);
 		if (new_array == NULL)
 			return -ENOMEM;
-		memcpy(new_array, *ea_inode_array,
-		       offsetof(struct ext4_xattr_inode_array, inodes[count]));
+		new_array->count = count;
+		memcpy(new_array->inodes, (*ea_inode_array)->inodes,
+		       count * sizeof(struct inode *));
 		kfree(*ea_inode_array);
 		*ea_inode_array = new_array;
 	}
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index bd97c4aa8177..e14fb19dc912 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -130,8 +130,8 @@ struct ext4_xattr_ibody_find {
 };
 
 struct ext4_xattr_inode_array {
-	unsigned int count;		/* # of used items in the array */
-	struct inode *inodes[];
+	unsigned int count;
+	struct inode *inodes[] __counted_by(count);
 };
 
 extern const struct xattr_handler ext4_xattr_user_handler;
-- 
2.45.2

Re: [PATCH v2] ext4: Annotate struct ext4_xattr_inode_array with __counted_by()

Posted by Gustavo A. R. Silva 1 year, 4 months ago


On 30/07/24 11:23, Thorsten Blum wrote:
> Add the __counted_by compiler attribute to the flexible array member
> inodes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Remove the now obsolete comment on the count field.
> 
> Refactor ext4_expand_inode_array() by assigning count before copying any
> data using memcpy(). Copy only the inodes array instead of the whole
> struct because count has been set explicitly.
> 
> Use struct_size() and struct_size_t() instead of offsetof().
> 
> Change the data type of the local variable count to unsigned int to
> match the struct's count data type.
> 
> Compile-tested only.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
> Changes in v2:
> - Adjust ext4_expand_inode_array() as suggested by Gustavo A. R. Silva
> - Use struct_size() and struct_size_t() instead of offsetof()
> - Link to v1: https://lore.kernel.org/linux-kernel/20240729110454.346918-3-thorsten.blum@toblux.com/
> ---
>   fs/ext4/xattr.c | 20 +++++++++-----------
>   fs/ext4/xattr.h |  4 ++--
>   2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 46ce2f21fef9..b27543587103 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2879,11 +2879,10 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>   	if (*ea_inode_array == NULL) {
>   		/*
>   		 * Start with 15 inodes, so it fits into a power-of-two size.
> -		 * If *ea_inode_array is NULL, this is essentially offsetof()
>   		 */
>   		(*ea_inode_array) =
> -			kmalloc(offsetof(struct ext4_xattr_inode_array,
> -					 inodes[EIA_MASK]),
> +			kmalloc(struct_size_t(struct ext4_xattr_inode_array,
> +					      inodes, EIA_MASK),

As Kees previously commented, you can use struct_size() here.

>   				GFP_NOFS);
>   		if (*ea_inode_array == NULL)
>   			return -ENOMEM;
> @@ -2891,17 +2890,16 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>   	} else if (((*ea_inode_array)->count & EIA_MASK) == EIA_MASK) {
>   		/* expand the array once all 15 + n * 16 slots are full */
>   		struct ext4_xattr_inode_array *new_array = NULL;
> -		int count = (*ea_inode_array)->count;
> +		unsigned int count = (*ea_inode_array)->count;

It seems `count` is not actually needed anymore.

If you remove it and directly use `(*ea_inode_array)->count` in the following
call to `kmalloc()`, you could use `struct_size()` in the call to `memcpy()`
below, and copy the whole thing in one line. See below.

>   
> -		/* if new_array is NULL, this is essentially offsetof() */
> -		new_array = kmalloc(
> -				offsetof(struct ext4_xattr_inode_array,
> -					 inodes[count + EIA_INCR]),
> -				GFP_NOFS);
> +		new_array = kmalloc(struct_size(*ea_inode_array, inodes,
> +						count + EIA_INCR),
> +				    GFP_NOFS);
>   		if (new_array == NULL)
>   			return -ENOMEM;
> -		memcpy(new_array, *ea_inode_array,
> -		       offsetof(struct ext4_xattr_inode_array, inodes[count]));
> +		new_array->count = count;
> +		memcpy(new_array->inodes, (*ea_inode_array)->inodes,
> +		       count * sizeof(struct inode *));

memcpy(new_array, *ea_inode_array, struct_size(new_array, inodes, (*ea_inode_array)->count));

>   		kfree(*ea_inode_array);
>   		*ea_inode_array = new_array;
>   	}

Also, you are missing one more like just below this one, where `(*ea_inode_array)->count`
is currently being used to directly index `inodes`:

(*ea_inode_array)->inodes[(*ea_inode_array)->count++] = inode;

Thanks
--
Gustavo

> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index bd97c4aa8177..e14fb19dc912 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -130,8 +130,8 @@ struct ext4_xattr_ibody_find {
>   };
>   
>   struct ext4_xattr_inode_array {
> -	unsigned int count;		/* # of used items in the array */
> -	struct inode *inodes[];
> +	unsigned int count;
> +	struct inode *inodes[] __counted_by(count);
>   };
>   
>   extern const struct xattr_handler ext4_xattr_user_handler;

Re: [PATCH v2] ext4: Annotate struct ext4_xattr_inode_array with __counted_by()

Posted by Thorsten Blum 1 year, 4 months ago

On 30. Jul 2024, at 20:12, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
> On 30/07/24 11:23, Thorsten Blum wrote:
>> Add the __counted_by compiler attribute to the flexible array member
>> inodes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>> CONFIG_FORTIFY_SOURCE.
>> Remove the now obsolete comment on the count field.
>> Refactor ext4_expand_inode_array() by assigning count before copying any
>> data using memcpy(). Copy only the inodes array instead of the whole
>> struct because count has been set explicitly.
>> Use struct_size() and struct_size_t() instead of offsetof().
>> Change the data type of the local variable count to unsigned int to
>> match the struct's count data type.
>> Compile-tested only.
>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
>> ---
>> Changes in v2:
>> - Adjust ext4_expand_inode_array() as suggested by Gustavo A. R. Silva
>> - Use struct_size() and struct_size_t() instead of offsetof()
>> - Link to v1: https://lore.kernel.org/linux-kernel/20240729110454.346918-3-thorsten.blum@toblux.com/
>> ---
>>  fs/ext4/xattr.c | 20 +++++++++-----------
>>  fs/ext4/xattr.h |  4 ++--
>>  2 files changed, 11 insertions(+), 13 deletions(-)
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index 46ce2f21fef9..b27543587103 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -2879,11 +2879,10 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>>   if (*ea_inode_array == NULL) {
>>   /*
>>   * Start with 15 inodes, so it fits into a power-of-two size.
>> - * If *ea_inode_array is NULL, this is essentially offsetof()
>>   */
>>   (*ea_inode_array) =
>> - kmalloc(offsetof(struct ext4_xattr_inode_array,
>> - inodes[EIA_MASK]),
>> + kmalloc(struct_size_t(struct ext4_xattr_inode_array,
>> +      inodes, EIA_MASK),
> 
> As Kees previously commented, you can use struct_size() here.
> 
>>   GFP_NOFS);
>>   if (*ea_inode_array == NULL)
>>   return -ENOMEM;
>> @@ -2891,17 +2890,16 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>>   } else if (((*ea_inode_array)->count & EIA_MASK) == EIA_MASK) {
>>   /* expand the array once all 15 + n * 16 slots are full */
>>   struct ext4_xattr_inode_array *new_array = NULL;
>> - int count = (*ea_inode_array)->count;
>> + unsigned int count = (*ea_inode_array)->count;
> 
> It seems `count` is not actually needed anymore.
> 
> If you remove it and directly use `(*ea_inode_array)->count` in the following
> call to `kmalloc()`, you could use `struct_size()` in the call to `memcpy()`
> below, and copy the whole thing in one line. See below.
> 
>>  - /* if new_array is NULL, this is essentially offsetof() */
>> - new_array = kmalloc(
>> - offsetof(struct ext4_xattr_inode_array,
>> - inodes[count + EIA_INCR]),
>> - GFP_NOFS);
>> + new_array = kmalloc(struct_size(*ea_inode_array, inodes,
>> + count + EIA_INCR),
>> +    GFP_NOFS);
>>   if (new_array == NULL)
>>   return -ENOMEM;
>> - memcpy(new_array, *ea_inode_array,
>> -       offsetof(struct ext4_xattr_inode_array, inodes[count]));
>> + new_array->count = count;
>> + memcpy(new_array->inodes, (*ea_inode_array)->inodes,
>> +       count * sizeof(struct inode *));
> 
> memcpy(new_array, *ea_inode_array, struct_size(new_array, inodes, (*ea_inode_array)->count));
> 
>>   kfree(*ea_inode_array);
>>   *ea_inode_array = new_array;
>>   }
> 
> Also, you are missing one more like just below this one, where `(*ea_inode_array)->count`
> is currently being used to directly index `inodes`:
> 
> (*ea_inode_array)->inodes[(*ea_inode_array)->count++] = inode;

Thanks, I missed this one.

Thorsten

Re: [PATCH v2] ext4: Annotate struct ext4_xattr_inode_array with __counted_by()

Posted by Kees Cook 1 year, 4 months ago

On Tue, Jul 30, 2024 at 07:23:04PM +0200, Thorsten Blum wrote:
> Add the __counted_by compiler attribute to the flexible array member
> inodes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Remove the now obsolete comment on the count field.
> 
> Refactor ext4_expand_inode_array() by assigning count before copying any
> data using memcpy(). Copy only the inodes array instead of the whole
> struct because count has been set explicitly.
> 
> Use struct_size() and struct_size_t() instead of offsetof().
> 
> Change the data type of the local variable count to unsigned int to
> match the struct's count data type.
> 
> Compile-tested only.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
> Changes in v2:
> - Adjust ext4_expand_inode_array() as suggested by Gustavo A. R. Silva
> - Use struct_size() and struct_size_t() instead of offsetof()
> - Link to v1: https://lore.kernel.org/linux-kernel/20240729110454.346918-3-thorsten.blum@toblux.com/
> ---
>  fs/ext4/xattr.c | 20 +++++++++-----------
>  fs/ext4/xattr.h |  4 ++--
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 46ce2f21fef9..b27543587103 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2879,11 +2879,10 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>  	if (*ea_inode_array == NULL) {
>  		/*
>  		 * Start with 15 inodes, so it fits into a power-of-two size.
> -		 * If *ea_inode_array is NULL, this is essentially offsetof()
>  		 */
>  		(*ea_inode_array) =
> -			kmalloc(offsetof(struct ext4_xattr_inode_array,
> -					 inodes[EIA_MASK]),
> +			kmalloc(struct_size_t(struct ext4_xattr_inode_array,

This can be struct_size(*ea_inode_array, ...) too, like you have below.

> +					      inodes, EIA_MASK),
>  				GFP_NOFS);
>  		if (*ea_inode_array == NULL)
>  			return -ENOMEM;
> @@ -2891,17 +2890,16 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array,
>  	} else if (((*ea_inode_array)->count & EIA_MASK) == EIA_MASK) {
>  		/* expand the array once all 15 + n * 16 slots are full */
>  		struct ext4_xattr_inode_array *new_array = NULL;
> -		int count = (*ea_inode_array)->count;
> +		unsigned int count = (*ea_inode_array)->count;
>  
> -		/* if new_array is NULL, this is essentially offsetof() */
> -		new_array = kmalloc(
> -				offsetof(struct ext4_xattr_inode_array,
> -					 inodes[count + EIA_INCR]),
> -				GFP_NOFS);
> +		new_array = kmalloc(struct_size(*ea_inode_array, inodes,
> +						count + EIA_INCR),
> +				    GFP_NOFS);
>  		if (new_array == NULL)
>  			return -ENOMEM;
> -		memcpy(new_array, *ea_inode_array,
> -		       offsetof(struct ext4_xattr_inode_array, inodes[count]));
> +		new_array->count = count;
> +		memcpy(new_array->inodes, (*ea_inode_array)->inodes,
> +		       count * sizeof(struct inode *));
>  		kfree(*ea_inode_array);
>  		*ea_inode_array = new_array;
>  	}
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index bd97c4aa8177..e14fb19dc912 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -130,8 +130,8 @@ struct ext4_xattr_ibody_find {
>  };
>  
>  struct ext4_xattr_inode_array {
> -	unsigned int count;		/* # of used items in the array */
> -	struct inode *inodes[];
> +	unsigned int count;
> +	struct inode *inodes[] __counted_by(count);
>  };
>  
>  extern const struct xattr_handler ext4_xattr_user_handler;

Thanks for the reworking!

-- 
Kees Cook