[PATCH -next] fs-verity: Use struct_size() helper in fsverity_ioctl_measure()

[PATCH -next] fs-verity: Use struct_size() helper in fsverity_ioctl_measure()

[Zhang Jianhua]

Make use of the struct_size() helper instead of an open-coded version,
in order to avoid any potential type mistakes or integer overflows that,
in the worst scenario, could lead to heap overflows.

Also, address the following sparse warnings:
fs/verity/measure.c:48:9: warning: using sizeof on a flexible structure
fs/verity/measure.c:52:38: warning: using sizeof on a flexible structure

Signed-off-by: Zhang Jianhua <chris.zjh@huawei.com>
---
 fs/verity/measure.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index e99c00350c28..4a388116d0de 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -27,6 +27,7 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
 	const struct fsverity_info *vi;
 	const struct fsverity_hash_alg *hash_alg;
 	struct fsverity_digest arg;
+	size_t arg_size = struct_size(&arg, digest, 0);
 
 	vi = fsverity_get_info(inode);
 	if (!vi)
@@ -44,11 +45,11 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
 	if (arg.digest_size < hash_alg->digest_size)
 		return -EOVERFLOW;
 
-	memset(&arg, 0, sizeof(arg));
+	memset(&arg, 0, arg_size);
 	arg.digest_algorithm = hash_alg - fsverity_hash_algs;
 	arg.digest_size = hash_alg->digest_size;
 
-	if (copy_to_user(uarg, &arg, sizeof(arg)))
+	if (copy_to_user(uarg, &arg, arg_size))
 		return -EFAULT;
 
 	if (copy_to_user(uarg->digest, vi->file_digest, hash_alg->digest_size))
-- 
2.31.0

Re: [PATCH -next] fs-verity: Use struct_size() helper in fsverity_ioctl_measure()

[Eric Biggers]

On Wed, May 18, 2022 at 05:38:29PM +0800, Zhang Jianhua wrote:
> Make use of the struct_size() helper instead of an open-coded version,
> in order to avoid any potential type mistakes or integer overflows that,
> in the worst scenario, could lead to heap overflows.
> 
> Also, address the following sparse warnings:
> fs/verity/measure.c:48:9: warning: using sizeof on a flexible structure
> fs/verity/measure.c:52:38: warning: using sizeof on a flexible structure
> 
> Signed-off-by: Zhang Jianhua <chris.zjh@huawei.com>
> ---
>  fs/verity/measure.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/verity/measure.c b/fs/verity/measure.c
> index e99c00350c28..4a388116d0de 100644
> --- a/fs/verity/measure.c
> +++ b/fs/verity/measure.c
> @@ -27,6 +27,7 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
>  	const struct fsverity_info *vi;
>  	const struct fsverity_hash_alg *hash_alg;
>  	struct fsverity_digest arg;
> +	size_t arg_size = struct_size(&arg, digest, 0);
>  
>  	vi = fsverity_get_info(inode);
>  	if (!vi)
> @@ -44,11 +45,11 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
>  	if (arg.digest_size < hash_alg->digest_size)
>  		return -EOVERFLOW;
>  
> -	memset(&arg, 0, sizeof(arg));
> +	memset(&arg, 0, arg_size);
>  	arg.digest_algorithm = hash_alg - fsverity_hash_algs;
>  	arg.digest_size = hash_alg->digest_size;
>  
> -	if (copy_to_user(uarg, &arg, sizeof(arg)))
> +	if (copy_to_user(uarg, &arg, arg_size))
>  		return -EFAULT;

'arg' is just a stack variable that doesn't use the flexible array field.  So
this change on its own is pretty pointless and just obfuscates the code.

If it's nevertheless worth it to get rid of the sparse warning, to make the
wider codebase clean of this class of warning, we could still do it anyway.  But
please make the commit message correctly say that the purpose is just to
eliminate the sparse warning, and don't incorrectly claim that the code "could
lead to heap overflows".

- Eric

Re: [PATCH -next] fs-verity: Use struct_size() helper in fsverity_ioctl_measure()

[zhangjianhua (E)]

Thanks, I will modify the commit message and send the next version.


Zhang Jianhua

在 2022/5/19 1:48, Eric Biggers 写道:
> On Wed, May 18, 2022 at 05:38:29PM +0800, Zhang Jianhua wrote:
>> Make use of the struct_size() helper instead of an open-coded version,
>> in order to avoid any potential type mistakes or integer overflows that,
>> in the worst scenario, could lead to heap overflows.
>>
>> Also, address the following sparse warnings:
>> fs/verity/measure.c:48:9: warning: using sizeof on a flexible structure
>> fs/verity/measure.c:52:38: warning: using sizeof on a flexible structure
>>
>> Signed-off-by: Zhang Jianhua <chris.zjh@huawei.com>
>> ---
>>   fs/verity/measure.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/verity/measure.c b/fs/verity/measure.c
>> index e99c00350c28..4a388116d0de 100644
>> --- a/fs/verity/measure.c
>> +++ b/fs/verity/measure.c
>> @@ -27,6 +27,7 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
>>   	const struct fsverity_info *vi;
>>   	const struct fsverity_hash_alg *hash_alg;
>>   	struct fsverity_digest arg;
>> +	size_t arg_size = struct_size(&arg, digest, 0);
>>   
>>   	vi = fsverity_get_info(inode);
>>   	if (!vi)
>> @@ -44,11 +45,11 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
>>   	if (arg.digest_size < hash_alg->digest_size)
>>   		return -EOVERFLOW;
>>   
>> -	memset(&arg, 0, sizeof(arg));
>> +	memset(&arg, 0, arg_size);
>>   	arg.digest_algorithm = hash_alg - fsverity_hash_algs;
>>   	arg.digest_size = hash_alg->digest_size;
>>   
>> -	if (copy_to_user(uarg, &arg, sizeof(arg)))
>> +	if (copy_to_user(uarg, &arg, arg_size))
>>   		return -EFAULT;
> 'arg' is just a stack variable that doesn't use the flexible array field.  So
> this change on its own is pretty pointless and just obfuscates the code.
>
> If it's nevertheless worth it to get rid of the sparse warning, to make the
> wider codebase clean of this class of warning, we could still do it anyway.  But
> please make the commit message correctly say that the purpose is just to
> eliminate the sparse warning, and don't incorrectly claim that the code "could
> lead to heap overflows".
>
> - Eric
> .