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

[Li Lingfeng <lilingfeng3@huawei.com>]

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;