Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 10:48:15 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Zhang Jianhua <chris.zjh@huawei.com>
Cc:     tytso@mit.edu, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] fs-verity: Use struct_size() helper in
 fsverity_ioctl_measure()
Message-ID: <YoUxX7iDBczYwGHC@sol.localdomain>
References: <20220518093829.2248801-1-chris.zjh@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220518093829.2248801-1-chris.zjh@huawei.com>
Precedence: bulk

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