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@xxxxxxxxxx>
>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> Reviewed-by: Anna Schumaker <anna.schumaker@xxxxxxxxxx>
>> Reviewed-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
>> ---
>> 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;
