Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0002ef8b-ebce-4b4b-8597-9687722d4e55@huawei.com>
Date:   Thu, 19 May 2022 09:50:38 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [PATCH -next] fs-verity: Use struct_size() helper in
 fsverity_ioctl_measure()
To:     Eric Biggers <ebiggers@kernel.org>
CC:     <tytso@mit.edu>, <linux-fscrypt@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20220518093829.2248801-1-chris.zjh@huawei.com>
 <YoUxX7iDBczYwGHC@sol.localdomain>
From:   "zhangjianhua (E)" <chris.zjh@huawei.com>
In-Reply-To: <YoUxX7iDBczYwGHC@sol.localdomain>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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