linux-kernel - Re: [PATCH -next] fs-verity: Use struct_size() helper in fsverity_ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 May 2022 10:48:15 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Zhang Jianhua <chris.zjh@...wei.com>
Cc:     tytso@....edu, linux-fscrypt@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] fs-verity: Use struct_size() helper in
 fsverity_ioctl_measure()

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