Re: [PATCH V2] nfs:check for user input filehandle size

["zhangjian (CG)" <zhangjian496@huawei.com>]

Sorry，I don't know reviewd-by is not allowed when there exists changes.

server_fh is obtained from nfs_exp_embedfh(fid->raw) but nfs_exp_embedfh
may be changed. Calculate by hand may be more safer.

fid->raw memory is copy_from_user in handle_to_path, which guarantee
[fid->raw: fid->raw + 4*fh_len ] memory is copied from userspace. So it
can be reliable.

Thanks for your criticism and guidance.

On 2025/6/26 14:31, Li Lingfeng wrote:
> Hi, Zhang Jian
> 
> server_fh is obtained via (struct nfs_fh *)(p + EMBED_FH_OFF). Shouldn't
> the condition (char*)server_fh <= (char*)p always be false?
> Additionally, (u32*)server_fh - (u32*)p + 1 appears to be a fixed value.
> Why use such an expression?
> Finally, fh_len is derived from user-provided handle->handle_bytes. Is
> this reliable?
> 
> By the way, you shouldn't add Anna and Benjamin's Reviewd-by, because they
> haven't seen this version of your changes, and they also have some
> comments on your previous version of changes. Also, Jeff only gave
> Reviewd-by for your previous version of changes, and your new version of
> changes is different from the previous one, so you shouldn't add it.
> 
> Thanks,
> Lingfeng.
> 在 2025/6/27 4:20, zhangjian 写道:
>> Syzkaller found an slab-out-of-bounds in nfs_fh_to_dentry when the memory
>> of server_fh is not passed from user space. So I add a check for input
>> size.
>>
>> Log is snipped as following:
>>
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in nfs_fh_to_dentry+0x4ad/0x6d0 fs/nfs/
>> export.c:70
>> Read of size 2 at addr ffff888100b9ffd4 by task syz-executor301/755
>>
>> CPU: 1 PID: 755 Comm: syz-executor301 Tainted: G        W        
>> 5.10.0 #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> 1.13.0-1ubuntu1.1 04/01/2014
>> Call Trace:
>>   __dump_stack lib/dump_stack.c:82 [inline]
>>   dump_stack+0x107/0x167 lib/dump_stack.c:123
>>   print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400
>>   __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560
>>   kasan_report+0x3a/0x50 mm/kasan/report.c:585
>>   nfs_fh_to_dentry+0x4ad/0x6d0 fs/nfs/export.c:70
>>   exportfs_decode_fh_raw+0x128/0x680 fs/exportfs/expfs.c:436
>>   exportfs_decode_fh+0x3d/0x90 fs/exportfs/expfs.c:575
>>   do_handle_to_path fs/fhandle.c:152 [inline]
>>   handle_to_path fs/fhandle.c:207 [inline]
>>   do_handle_open+0x2c3/0x8d0 fs/fhandle.c:223
>>   do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
>>   entry_SYSCALL_64_after_hwframe+0x67/0xd1
>> ==================================================================
>>
>> Signed-off-by: zhangjian <zhangjian496@huawei.com>
>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>> Reviewed-by: Anna Schumaker <anna.schumaker@oracle.com>
>> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>   fs/nfs/export.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/export.c b/fs/nfs/export.c
>> index e9c233b6f..565e01788 100644
>> --- a/fs/nfs/export.c
>> +++ b/fs/nfs/export.c
>> @@ -66,14 +66,21 @@ nfs_fh_to_dentry(struct super_block *sb, struct
>> fid *fid,
>>   {
>>       struct nfs_fattr *fattr = NULL;
>>       struct nfs_fh *server_fh = nfs_exp_embedfh(fid->raw);
>> -    size_t fh_size = offsetof(struct nfs_fh, data) + server_fh->size;
>> +    size_t fh_size;
>>       const struct nfs_rpc_ops *rpc_ops;
>>       struct dentry *dentry;
>>       struct inode *inode;
>> -    int len = EMBED_FH_OFF + XDR_QUADLEN(fh_size);
>> +    int len;
>>       u32 *p = fid->raw;
>>       int ret;
>>   +    /* check for user input size */
>> +    if ((char*)server_fh <= (char*)p || (int)((u32*)server_fh -
>> (u32*)p + 1) < fh_len)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    fh_size = offsetof(struct nfs_fh, data) + server_fh->size;
>> +    len = EMBED_FH_OFF + XDR_QUADLEN(fh_size);
>> +
>>       /* NULL translates to ESTALE */
>>       if (fh_len < len || fh_type != len)
>>           return NULL;