* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 11:14 [PATCH] nfsd: return -EINVAL when namelen is 0 Li Lingfeng
@ 2024-09-03 11:23 ` Jeff Layton
2024-09-03 15:27 ` Chuck Lever
2024-09-03 21:35 ` Chuck Lever
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2024-09-03 11:23 UTC (permalink / raw)
To: Li Lingfeng, chuck.lever, neilb, okorniev, Dai.Ngo, tom
Cc: linux-nfs, linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun,
lilingfeng
On Tue, 2024-09-03 at 19:14 +0800, Li Lingfeng wrote:
> When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> result in namelen being 0, which will cause memdup_user() to return
> ZERO_SIZE_PTR.
> When we access the name.data that has been assigned the value of
> ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> triggered.
>
> [ T1205] ==================================================================
> [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> [ T1205]
> [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> [ T1205] Call Trace:
> [ T1205] dump_stack+0x9a/0xd0
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] __kasan_report.cold+0x34/0x84
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] kasan_report+0x3a/0x50
> [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> [ T1205] cld_pipe_downcall+0x5ca/0x760
> [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> [ T1205] ? down_write_killable_nested+0x170/0x170
> [ T1205] ? avc_policy_seqno+0x28/0x40
> [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> [ T1205] rpc_pipe_write+0x84/0xb0
> [ T1205] vfs_write+0x143/0x520
> [ T1205] ksys_write+0xc9/0x170
> [ T1205] ? __ia32_sys_read+0x50/0x50
> [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> [ T1205] do_syscall_64+0x33/0x40
> [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> [ T1205] RIP: 0033:0x7fdbdb761bc7
> [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> [ T1205] ==================================================================
>
> Fix it by checking namelen.
>
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
> fs/nfsd/nfs4recover.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 67d8673a9391..69a3a84e159e 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> ci = &cmsg->cm_u.cm_clntinfo;
> if (get_user(namelen, &ci->cc_name.cn_len))
> return -EFAULT;
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
> @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> cnm = &cmsg->cm_u.cm_name;
> if (get_user(namelen, &cnm->cn_len))
> return -EFAULT;
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&cnm->cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
This error will come back to nfsdcld in its downcall write(). -EINVAL
is a bit generic. It might be better to go with a more distinct error,
but I don't think it matters too much.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 11:23 ` Jeff Layton
@ 2024-09-03 15:27 ` Chuck Lever
2024-09-03 15:35 ` Jeff Layton
0 siblings, 1 reply; 16+ messages in thread
From: Chuck Lever @ 2024-09-03 15:27 UTC (permalink / raw)
To: Jeff Layton, Scott Mayhew
Cc: Li Lingfeng, neilb, okorniev, Dai.Ngo, tom, linux-nfs,
linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
On Tue, Sep 03, 2024 at 07:23:18AM -0400, Jeff Layton wrote:
> On Tue, 2024-09-03 at 19:14 +0800, Li Lingfeng wrote:
> > When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> > result in namelen being 0, which will cause memdup_user() to return
> > ZERO_SIZE_PTR.
> > When we access the name.data that has been assigned the value of
> > ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> > triggered.
> >
> > [ T1205] ==================================================================
> > [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> > [ T1205]
> > [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> > [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> > [ T1205] Call Trace:
> > [ T1205] dump_stack+0x9a/0xd0
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] __kasan_report.cold+0x34/0x84
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] kasan_report+0x3a/0x50
> > [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> > [ T1205] cld_pipe_downcall+0x5ca/0x760
> > [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> > [ T1205] ? down_write_killable_nested+0x170/0x170
> > [ T1205] ? avc_policy_seqno+0x28/0x40
> > [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> > [ T1205] rpc_pipe_write+0x84/0xb0
> > [ T1205] vfs_write+0x143/0x520
> > [ T1205] ksys_write+0xc9/0x170
> > [ T1205] ? __ia32_sys_read+0x50/0x50
> > [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> > [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> > [ T1205] do_syscall_64+0x33/0x40
> > [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > [ T1205] RIP: 0033:0x7fdbdb761bc7
> > [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> > [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> > [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> > [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> > [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> > [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> > [ T1205] ==================================================================
> >
> > Fix it by checking namelen.
> >
> > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > ---
> > fs/nfsd/nfs4recover.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index 67d8673a9391..69a3a84e159e 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > ci = &cmsg->cm_u.cm_clntinfo;
> > if (get_user(namelen, &ci->cc_name.cn_len))
> > return -EFAULT;
> > + if (!namelen) {
> > + dprintk("%s: namelen should not be zero", __func__);
> > + return -EINVAL;
> > + }
> > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> > if (IS_ERR(name.data))
> > return PTR_ERR(name.data);
> > @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > cnm = &cmsg->cm_u.cm_name;
> > if (get_user(namelen, &cnm->cn_len))
> > return -EFAULT;
> > + if (!namelen) {
> > + dprintk("%s: namelen should not be zero", __func__);
> > + return -EINVAL;
> > + }
> > name.data = memdup_user(&cnm->cn_id, namelen);
> > if (IS_ERR(name.data))
> > return PTR_ERR(name.data);
>
> This error will come back to nfsdcld in its downcall write(). -EINVAL
> is a bit generic. It might be better to go with a more distinct error,
> but I don't think it matters too much.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
Wondering if
Fixes: 74725959c33c ("nfsd: un-deprecate nfsdcld")
would be appropriate.
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 15:27 ` Chuck Lever
@ 2024-09-03 15:35 ` Jeff Layton
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2024-09-03 15:35 UTC (permalink / raw)
To: Chuck Lever, Scott Mayhew
Cc: Li Lingfeng, neilb, okorniev, Dai.Ngo, tom, linux-nfs,
linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
On Tue, 2024-09-03 at 11:27 -0400, Chuck Lever wrote:
> On Tue, Sep 03, 2024 at 07:23:18AM -0400, Jeff Layton wrote:
> > On Tue, 2024-09-03 at 19:14 +0800, Li Lingfeng wrote:
> > > When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> > > result in namelen being 0, which will cause memdup_user() to return
> > > ZERO_SIZE_PTR.
> > > When we access the name.data that has been assigned the value of
> > > ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> > > triggered.
> > >
> > > [ T1205] ==================================================================
> > > [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> > > [ T1205]
> > > [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> > > [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> > > [ T1205] Call Trace:
> > > [ T1205] dump_stack+0x9a/0xd0
> > > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] __kasan_report.cold+0x34/0x84
> > > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] kasan_report+0x3a/0x50
> > > [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> > > [ T1205] cld_pipe_downcall+0x5ca/0x760
> > > [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> > > [ T1205] ? down_write_killable_nested+0x170/0x170
> > > [ T1205] ? avc_policy_seqno+0x28/0x40
> > > [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> > > [ T1205] rpc_pipe_write+0x84/0xb0
> > > [ T1205] vfs_write+0x143/0x520
> > > [ T1205] ksys_write+0xc9/0x170
> > > [ T1205] ? __ia32_sys_read+0x50/0x50
> > > [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> > > [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> > > [ T1205] do_syscall_64+0x33/0x40
> > > [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > > [ T1205] RIP: 0033:0x7fdbdb761bc7
> > > [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> > > [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> > > [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> > > [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> > > [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> > > [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> > > [ T1205] ==================================================================
> > >
> > > Fix it by checking namelen.
> > >
> > > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > > ---
> > > fs/nfsd/nfs4recover.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > index 67d8673a9391..69a3a84e159e 100644
> > > --- a/fs/nfsd/nfs4recover.c
> > > +++ b/fs/nfsd/nfs4recover.c
> > > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > ci = &cmsg->cm_u.cm_clntinfo;
> > > if (get_user(namelen, &ci->cc_name.cn_len))
> > > return -EFAULT;
> > > + if (!namelen) {
> > > + dprintk("%s: namelen should not be zero", __func__);
> > > + return -EINVAL;
> > > + }
> > > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> > > if (IS_ERR(name.data))
> > > return PTR_ERR(name.data);
> > > @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > cnm = &cmsg->cm_u.cm_name;
> > > if (get_user(namelen, &cnm->cn_len))
> > > return -EFAULT;
> > > + if (!namelen) {
> > > + dprintk("%s: namelen should not be zero", __func__);
> > > + return -EINVAL;
> > > + }
> > > name.data = memdup_user(&cnm->cn_id, namelen);
> > > if (IS_ERR(name.data))
> > > return PTR_ERR(name.data);
> >
> > This error will come back to nfsdcld in its downcall write(). -EINVAL
> > is a bit generic. It might be better to go with a more distinct error,
> > but I don't think it matters too much.
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> Wondering if
>
> Fixes: 74725959c33c ("nfsd: un-deprecate nfsdcld")
>
> would be appropriate.
>
>
Hmm, 2019 patch. Are there any supported release streams that don't
have that commit?
Oh well, I guess it won't hurt anything to add it.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 11:14 [PATCH] nfsd: return -EINVAL when namelen is 0 Li Lingfeng
2024-09-03 11:23 ` Jeff Layton
@ 2024-09-03 21:35 ` Chuck Lever
2024-09-04 13:06 ` Guoqing Jiang
2024-09-04 14:48 ` Scott Mayhew
3 siblings, 0 replies; 16+ messages in thread
From: Chuck Lever @ 2024-09-03 21:35 UTC (permalink / raw)
To: Li Lingfeng
Cc: jlayton, neilb, okorniev, Dai.Ngo, tom, linux-nfs, linux-kernel,
yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
On Tue, 03 Sep 2024 19:14:46 +0800, Li Lingfeng wrote:
> When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> result in namelen being 0, which will cause memdup_user() to return
> ZERO_SIZE_PTR.
> When we access the name.data that has been assigned the value of
> ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> triggered.
>
> [...]
Kept the new dprintk call sites since this is not a hot path and
there needs to be some observability here rather than a silent
failure. I'm not convinced the error text is especially clear, but
I don't have a better suggestion at the moment.
Applied to nfsd-next for v6.12, thanks!
[1/1] nfsd: return -EINVAL when namelen is 0
commit: e492841732bbce2b2dd19cd285d5e7f61b1bdaee
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 11:14 [PATCH] nfsd: return -EINVAL when namelen is 0 Li Lingfeng
2024-09-03 11:23 ` Jeff Layton
2024-09-03 21:35 ` Chuck Lever
@ 2024-09-04 13:06 ` Guoqing Jiang
2024-09-04 14:16 ` Chuck Lever
2024-09-04 14:48 ` Scott Mayhew
3 siblings, 1 reply; 16+ messages in thread
From: Guoqing Jiang @ 2024-09-04 13:06 UTC (permalink / raw)
To: Li Lingfeng, chuck.lever, jlayton, neilb, okorniev, Dai.Ngo, tom
Cc: linux-nfs, linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun,
lilingfeng
On 9/3/24 19:14, Li Lingfeng wrote:
> When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> result in namelen being 0, which will cause memdup_user() to return
> ZERO_SIZE_PTR.
> When we access the name.data that has been assigned the value of
> ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> triggered.
>
> [ T1205] ==================================================================
> [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> [ T1205]
> [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> [ T1205] Call Trace:
> [ T1205] dump_stack+0x9a/0xd0
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] __kasan_report.cold+0x34/0x84
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] kasan_report+0x3a/0x50
> [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> [ T1205] cld_pipe_downcall+0x5ca/0x760
> [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> [ T1205] ? down_write_killable_nested+0x170/0x170
> [ T1205] ? avc_policy_seqno+0x28/0x40
> [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> [ T1205] rpc_pipe_write+0x84/0xb0
> [ T1205] vfs_write+0x143/0x520
> [ T1205] ksys_write+0xc9/0x170
> [ T1205] ? __ia32_sys_read+0x50/0x50
> [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> [ T1205] do_syscall_64+0x33/0x40
> [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> [ T1205] RIP: 0033:0x7fdbdb761bc7
> [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> [ T1205] ==================================================================
>
> Fix it by checking namelen.
>
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
> fs/nfsd/nfs4recover.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 67d8673a9391..69a3a84e159e 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> ci = &cmsg->cm_u.cm_clntinfo;
> if (get_user(namelen, &ci->cc_name.cn_len))
> return -EFAULT;
If namelen is 0, I think the func should return -EFAULT above per below
comment in [1]. Or can
get_user return success with x was set to zero?
* Return: zero on success, or -EFAULT on error.
* On error, the variable @x is set to zero.
*/
#define get_user(x,ptr) ({ might_fault();
do_get_user_call(get_user,x,ptr); })
[1].
https://elixir.bootlin.com/linux/v6.11-rc6/source/arch/x86/include/asm/uaccess.h#L108
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
> @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> cnm = &cmsg->cm_u.cm_name;
> if (get_user(namelen, &cnm->cn_len))
> return -EFAULT;
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&cnm->cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-04 13:06 ` Guoqing Jiang
@ 2024-09-04 14:16 ` Chuck Lever
0 siblings, 0 replies; 16+ messages in thread
From: Chuck Lever @ 2024-09-04 14:16 UTC (permalink / raw)
To: Guoqing Jiang
Cc: Li Lingfeng, jlayton, neilb, okorniev, Dai.Ngo, tom, linux-nfs,
linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
On Wed, Sep 04, 2024 at 09:06:07PM +0800, Guoqing Jiang wrote:
>
>
> On 9/3/24 19:14, Li Lingfeng wrote:
> > When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> > result in namelen being 0, which will cause memdup_user() to return
> > ZERO_SIZE_PTR.
> > When we access the name.data that has been assigned the value of
> > ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> > triggered.
> >
> > [ T1205] ==================================================================
> > [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> > [ T1205]
> > [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> > [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> > [ T1205] Call Trace:
> > [ T1205] dump_stack+0x9a/0xd0
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] __kasan_report.cold+0x34/0x84
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] kasan_report+0x3a/0x50
> > [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> > [ T1205] cld_pipe_downcall+0x5ca/0x760
> > [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> > [ T1205] ? down_write_killable_nested+0x170/0x170
> > [ T1205] ? avc_policy_seqno+0x28/0x40
> > [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> > [ T1205] rpc_pipe_write+0x84/0xb0
> > [ T1205] vfs_write+0x143/0x520
> > [ T1205] ksys_write+0xc9/0x170
> > [ T1205] ? __ia32_sys_read+0x50/0x50
> > [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> > [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> > [ T1205] do_syscall_64+0x33/0x40
> > [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > [ T1205] RIP: 0033:0x7fdbdb761bc7
> > [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> > [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> > [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> > [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> > [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> > [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> > [ T1205] ==================================================================
> >
> > Fix it by checking namelen.
> >
> > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > ---
> > fs/nfsd/nfs4recover.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index 67d8673a9391..69a3a84e159e 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > ci = &cmsg->cm_u.cm_clntinfo;
> > if (get_user(namelen, &ci->cc_name.cn_len))
> > return -EFAULT;
>
> If namelen is 0, I think the func should return -EFAULT above per below
> comment in [1]. Or can get_user return success with x was set to zero?
It's legitimate in general to find a zero stored at &ptr. x can also
be set to zero if a fault occurs.
Clearly, get_user() succeeds in the crashing case; otherwise the
flow cannot reach the memdup_user() call that returns ZERO_SIZE_PTR.
> * Return: zero on success, or -EFAULT on error.
> * On error, the variable @x is set to zero.
> */
> #define get_user(x,ptr) ({ might_fault(); do_get_user_call(get_user,x,ptr);
> })
>
> [1]. https://elixir.bootlin.com/linux/v6.11-rc6/source/arch/x86/include/asm/uaccess.h#L108
>
> > + if (!namelen) {
> > + dprintk("%s: namelen should not be zero", __func__);
> > + return -EINVAL;
> > + }
> > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> > if (IS_ERR(name.data))
> > return PTR_ERR(name.data);
> > @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > cnm = &cmsg->cm_u.cm_name;
> > if (get_user(namelen, &cnm->cn_len))
> > return -EFAULT;
> > + if (!namelen) {
> > + dprintk("%s: namelen should not be zero", __func__);
> > + return -EINVAL;
> > + }
> > name.data = memdup_user(&cnm->cn_id, namelen);
> > if (IS_ERR(name.data))
> > return PTR_ERR(name.data);
>
> Thanks,
> Guoqing
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-03 11:14 [PATCH] nfsd: return -EINVAL when namelen is 0 Li Lingfeng
` (2 preceding siblings ...)
2024-09-04 13:06 ` Guoqing Jiang
@ 2024-09-04 14:48 ` Scott Mayhew
2024-09-05 1:25 ` Li Lingfeng
2024-09-08 20:25 ` David Laight
3 siblings, 2 replies; 16+ messages in thread
From: Scott Mayhew @ 2024-09-04 14:48 UTC (permalink / raw)
To: Li Lingfeng
Cc: chuck.lever, jlayton, neilb, okorniev, Dai.Ngo, tom, linux-nfs,
linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
On Tue, 03 Sep 2024, Li Lingfeng wrote:
> When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> result in namelen being 0, which will cause memdup_user() to return
> ZERO_SIZE_PTR.
> When we access the name.data that has been assigned the value of
> ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> triggered.
>
> [ T1205] ==================================================================
> [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> [ T1205]
> [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> [ T1205] Call Trace:
> [ T1205] dump_stack+0x9a/0xd0
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] __kasan_report.cold+0x34/0x84
> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] kasan_report+0x3a/0x50
> [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> [ T1205] cld_pipe_downcall+0x5ca/0x760
> [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> [ T1205] ? down_write_killable_nested+0x170/0x170
> [ T1205] ? avc_policy_seqno+0x28/0x40
> [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> [ T1205] rpc_pipe_write+0x84/0xb0
> [ T1205] vfs_write+0x143/0x520
> [ T1205] ksys_write+0xc9/0x170
> [ T1205] ? __ia32_sys_read+0x50/0x50
> [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> [ T1205] do_syscall_64+0x33/0x40
> [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> [ T1205] RIP: 0033:0x7fdbdb761bc7
> [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> [ T1205] ==================================================================
>
> Fix it by checking namelen.
>
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
> fs/nfsd/nfs4recover.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 67d8673a9391..69a3a84e159e 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> ci = &cmsg->cm_u.cm_clntinfo;
> if (get_user(namelen, &ci->cc_name.cn_len))
> return -EFAULT;
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
> @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> cnm = &cmsg->cm_u.cm_name;
> if (get_user(namelen, &cnm->cn_len))
> return -EFAULT;
> + if (!namelen) {
> + dprintk("%s: namelen should not be zero", __func__);
> + return -EINVAL;
> + }
> name.data = memdup_user(&cnm->cn_id, namelen);
> if (IS_ERR(name.data))
> return PTR_ERR(name.data);
> --
> 2.31.1
Huh, so that would mean sqlite allows null in a primary key. Any idea
how the corruption occurs in the first place?
Reviewed-and-tested-by: Scott Mayhew <smayhew@redhat.com>
-Scott
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-04 14:48 ` Scott Mayhew
@ 2024-09-05 1:25 ` Li Lingfeng
2024-09-08 20:25 ` David Laight
1 sibling, 0 replies; 16+ messages in thread
From: Li Lingfeng @ 2024-09-05 1:25 UTC (permalink / raw)
To: Scott Mayhew
Cc: chuck.lever, jlayton, neilb, okorniev, Dai.Ngo, tom, linux-nfs,
linux-kernel, yukuai1, houtao1, yi.zhang, yangerkun, lilingfeng
在 2024/9/4 22:48, Scott Mayhew 写道:
> On Tue, 03 Sep 2024, Li Lingfeng wrote:
>
>> When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
>> result in namelen being 0, which will cause memdup_user() to return
>> ZERO_SIZE_PTR.
>> When we access the name.data that has been assigned the value of
>> ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
>> triggered.
>>
>> [ T1205] ==================================================================
>> [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
>> [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
>> [ T1205]
>> [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
>> [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
>> [ T1205] Call Trace:
>> [ T1205] dump_stack+0x9a/0xd0
>> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
>> [ T1205] __kasan_report.cold+0x34/0x84
>> [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
>> [ T1205] kasan_report+0x3a/0x50
>> [ T1205] nfs4_client_to_reclaim+0xe9/0x260
>> [ T1205] ? nfsd4_release_lockowner+0x410/0x410
>> [ T1205] cld_pipe_downcall+0x5ca/0x760
>> [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
>> [ T1205] ? down_write_killable_nested+0x170/0x170
>> [ T1205] ? avc_policy_seqno+0x28/0x40
>> [ T1205] ? selinux_file_permission+0x1b4/0x1e0
>> [ T1205] rpc_pipe_write+0x84/0xb0
>> [ T1205] vfs_write+0x143/0x520
>> [ T1205] ksys_write+0xc9/0x170
>> [ T1205] ? __ia32_sys_read+0x50/0x50
>> [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
>> [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
>> [ T1205] do_syscall_64+0x33/0x40
>> [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
>> [ T1205] RIP: 0033:0x7fdbdb761bc7
>> [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
>> [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>> [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
>> [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
>> [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
>> [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
>> [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
>> [ T1205] ==================================================================
>>
>> Fix it by checking namelen.
>>
>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>> ---
>> fs/nfsd/nfs4recover.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
>> index 67d8673a9391..69a3a84e159e 100644
>> --- a/fs/nfsd/nfs4recover.c
>> +++ b/fs/nfsd/nfs4recover.c
>> @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
>> ci = &cmsg->cm_u.cm_clntinfo;
>> if (get_user(namelen, &ci->cc_name.cn_len))
>> return -EFAULT;
>> + if (!namelen) {
>> + dprintk("%s: namelen should not be zero", __func__);
>> + return -EINVAL;
>> + }
>> name.data = memdup_user(&ci->cc_name.cn_id, namelen);
>> if (IS_ERR(name.data))
>> return PTR_ERR(name.data);
>> @@ -831,6 +835,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
>> cnm = &cmsg->cm_u.cm_name;
>> if (get_user(namelen, &cnm->cn_len))
>> return -EFAULT;
>> + if (!namelen) {
>> + dprintk("%s: namelen should not be zero", __func__);
>> + return -EINVAL;
>> + }
>> name.data = memdup_user(&cnm->cn_id, namelen);
>> if (IS_ERR(name.data))
>> return PTR_ERR(name.data);
>> --
>> 2.31.1
> Huh, so that would mean sqlite allows null in a primary key. Any idea
> how the corruption occurs in the first place?
During the test on the VM, the machine was hung. After the VM was powered
off, the corrupted sqlite was obtained. But we don't know how to
theoretically trigger this corruption.
>
> Reviewed-and-tested-by: Scott Mayhew <smayhew@redhat.com>
>
> -Scott
>>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-04 14:48 ` Scott Mayhew
2024-09-05 1:25 ` Li Lingfeng
@ 2024-09-08 20:25 ` David Laight
2024-09-09 11:24 ` Scott Mayhew
1 sibling, 1 reply; 16+ messages in thread
From: David Laight @ 2024-09-08 20:25 UTC (permalink / raw)
To: 'Scott Mayhew', Li Lingfeng
Cc: chuck.lever@oracle.com, jlayton@kernel.org, neilb@suse.de,
okorniev@redhat.com, Dai.Ngo@oracle.com, tom@talpey.com,
linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org,
yukuai1@huaweicloud.com, houtao1@huawei.com, yi.zhang@huawei.com,
yangerkun@huawei.com, lilingfeng@huaweicloud.com
From: Scott Mayhew
> Sent: 04 September 2024 15:48
>
> On Tue, 03 Sep 2024, Li Lingfeng wrote:
>
> > When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> > result in namelen being 0, which will cause memdup_user() to return
> > ZERO_SIZE_PTR.
> > When we access the name.data that has been assigned the value of
> > ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> > triggered.
> >
> > [ T1205] ==================================================================
> > [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> > [ T1205]
> > [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> > [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-
> ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> > [ T1205] Call Trace:
> > [ T1205] dump_stack+0x9a/0xd0
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] __kasan_report.cold+0x34/0x84
> > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] kasan_report+0x3a/0x50
> > [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> > [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> > [ T1205] cld_pipe_downcall+0x5ca/0x760
> > [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> > [ T1205] ? down_write_killable_nested+0x170/0x170
> > [ T1205] ? avc_policy_seqno+0x28/0x40
> > [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> > [ T1205] rpc_pipe_write+0x84/0xb0
> > [ T1205] vfs_write+0x143/0x520
> > [ T1205] ksys_write+0xc9/0x170
> > [ T1205] ? __ia32_sys_read+0x50/0x50
> > [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> > [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> > [ T1205] do_syscall_64+0x33/0x40
> > [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > [ T1205] RIP: 0033:0x7fdbdb761bc7
> > [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18
> 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> > [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> > [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> > [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> > [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> > [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> > [ T1205] ==================================================================
> >
> > Fix it by checking namelen.
> >
> > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > ---
> > fs/nfsd/nfs4recover.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index 67d8673a9391..69a3a84e159e 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > ci = &cmsg->cm_u.cm_clntinfo;
> > if (get_user(namelen, &ci->cc_name.cn_len))
> > return -EFAULT;
> > + if (!namelen) {
> > + dprintk("%s: namelen should not be zero", __func__);
> > + return -EINVAL;
> > + }
> > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
Don't you also want an upper bound sanity check?
(or is cn_len only 8 bit?)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-08 20:25 ` David Laight
@ 2024-09-09 11:24 ` Scott Mayhew
2024-09-09 11:33 ` David Laight
0 siblings, 1 reply; 16+ messages in thread
From: Scott Mayhew @ 2024-09-09 11:24 UTC (permalink / raw)
To: David Laight
Cc: Li Lingfeng, chuck.lever@oracle.com, jlayton@kernel.org,
neilb@suse.de, okorniev@redhat.com, Dai.Ngo@oracle.com,
tom@talpey.com, linux-nfs@vger.kernel.org,
linux-kernel@vger.kernel.org, yukuai1@huaweicloud.com,
houtao1@huawei.com, yi.zhang@huawei.com, yangerkun@huawei.com,
lilingfeng@huaweicloud.com
On Sun, 08 Sep 2024, David Laight wrote:
> From: Scott Mayhew
> > Sent: 04 September 2024 15:48
> >
> > On Tue, 03 Sep 2024, Li Lingfeng wrote:
> >
> > > When we have a corrupted main.sqlite in /var/lib/nfs/nfsdcld/, it may
> > > result in namelen being 0, which will cause memdup_user() to return
> > > ZERO_SIZE_PTR.
> > > When we access the name.data that has been assigned the value of
> > > ZERO_SIZE_PTR in nfs4_client_to_reclaim(), null pointer dereference is
> > > triggered.
> > >
> > > [ T1205] ==================================================================
> > > [ T1205] BUG: KASAN: null-ptr-deref in nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] Read of size 1 at addr 0000000000000010 by task nfsdcld/1205
> > > [ T1205]
> > > [ T1205] CPU: 11 PID: 1205 Comm: nfsdcld Not tainted 5.10.0-00003-g2c1423731b8d #406
> > > [ T1205] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-
> > ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
> > > [ T1205] Call Trace:
> > > [ T1205] dump_stack+0x9a/0xd0
> > > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] __kasan_report.cold+0x34/0x84
> > > [ T1205] ? nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] kasan_report+0x3a/0x50
> > > [ T1205] nfs4_client_to_reclaim+0xe9/0x260
> > > [ T1205] ? nfsd4_release_lockowner+0x410/0x410
> > > [ T1205] cld_pipe_downcall+0x5ca/0x760
> > > [ T1205] ? nfsd4_cld_tracking_exit+0x1d0/0x1d0
> > > [ T1205] ? down_write_killable_nested+0x170/0x170
> > > [ T1205] ? avc_policy_seqno+0x28/0x40
> > > [ T1205] ? selinux_file_permission+0x1b4/0x1e0
> > > [ T1205] rpc_pipe_write+0x84/0xb0
> > > [ T1205] vfs_write+0x143/0x520
> > > [ T1205] ksys_write+0xc9/0x170
> > > [ T1205] ? __ia32_sys_read+0x50/0x50
> > > [ T1205] ? ktime_get_coarse_real_ts64+0xfe/0x110
> > > [ T1205] ? ktime_get_coarse_real_ts64+0xa2/0x110
> > > [ T1205] do_syscall_64+0x33/0x40
> > > [ T1205] entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > > [ T1205] RIP: 0033:0x7fdbdb761bc7
> > > [ T1205] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18
> > 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 514
> > > [ T1205] RSP: 002b:00007fff8c4b7248 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [ T1205] RAX: ffffffffffffffda RBX: 000000000000042b RCX: 00007fdbdb761bc7
> > > [ T1205] RDX: 000000000000042b RSI: 00007fff8c4b75f0 RDI: 0000000000000008
> > > [ T1205] RBP: 00007fdbdb761bb0 R08: 0000000000000000 R09: 0000000000000001
> > > [ T1205] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000042b
> > > [ T1205] R13: 0000000000000008 R14: 00007fff8c4b75f0 R15: 0000000000000000
> > > [ T1205] ==================================================================
> > >
> > > Fix it by checking namelen.
> > >
> > > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > > ---
> > > fs/nfsd/nfs4recover.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > index 67d8673a9391..69a3a84e159e 100644
> > > --- a/fs/nfsd/nfs4recover.c
> > > +++ b/fs/nfsd/nfs4recover.c
> > > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > ci = &cmsg->cm_u.cm_clntinfo;
> > > if (get_user(namelen, &ci->cc_name.cn_len))
> > > return -EFAULT;
> > > + if (!namelen) {
> > > + dprintk("%s: namelen should not be zero", __func__);
> > > + return -EINVAL;
> > > + }
> > > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
>
> Don't you also want an upper bound sanity check?
> (or is cn_len only 8 bit?)
Yeah, actually it should probably be checking for namelen >
NFS4_OPAQUE_LIMIT.
-Scott
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-09 11:24 ` Scott Mayhew
@ 2024-09-09 11:33 ` David Laight
2024-09-09 14:10 ` Chuck Lever
0 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2024-09-09 11:33 UTC (permalink / raw)
To: 'Scott Mayhew'
Cc: Li Lingfeng, chuck.lever@oracle.com, jlayton@kernel.org,
neilb@suse.de, okorniev@redhat.com, Dai.Ngo@oracle.com,
tom@talpey.com, linux-nfs@vger.kernel.org,
linux-kernel@vger.kernel.org, yukuai1@huaweicloud.com,
houtao1@huawei.com, yi.zhang@huawei.com, yangerkun@huawei.com,
lilingfeng@huaweicloud.com
From: Scott Mayhew
> Sent: 09 September 2024 12:24
...
> > > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > > index 67d8673a9391..69a3a84e159e 100644
> > > > --- a/fs/nfsd/nfs4recover.c
> > > > +++ b/fs/nfsd/nfs4recover.c
> > > > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > > ci = &cmsg->cm_u.cm_clntinfo;
> > > > if (get_user(namelen, &ci->cc_name.cn_len))
> > > > return -EFAULT;
> > > > + if (!namelen) {
> > > > + dprintk("%s: namelen should not be zero", __func__);
> > > > + return -EINVAL;
> > > > + }
> > > > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> >
> > Don't you also want an upper bound sanity check?
> > (or is cn_len only 8 bit?)
>
> Yeah, actually it should probably be checking for namelen >
> NFS4_OPAQUE_LIMIT.
I suspect memdup_user() itself should have a third 'maxlen' argument.
And probably one that is required to be a compile-time constant.
Oh, and is dprintk() rate-limited?
Not that the message looks very helpful.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] nfsd: return -EINVAL when namelen is 0
2024-09-09 11:33 ` David Laight
@ 2024-09-09 14:10 ` Chuck Lever
2024-09-09 20:28 ` [PATCH 0/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall() Scott Mayhew
0 siblings, 1 reply; 16+ messages in thread
From: Chuck Lever @ 2024-09-09 14:10 UTC (permalink / raw)
To: David Laight, Scott Mayhew
Cc: Li Lingfeng, jlayton@kernel.org, neilb@suse.de,
okorniev@redhat.com, Dai.Ngo@oracle.com, tom@talpey.com,
linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org,
yukuai1@huaweicloud.com, houtao1@huawei.com, yi.zhang@huawei.com,
yangerkun@huawei.com, lilingfeng@huaweicloud.com
On Mon, Sep 09, 2024 at 11:33:04AM +0000, David Laight wrote:
> From: Scott Mayhew
> > Sent: 09 September 2024 12:24
> ...
> > > > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > > > index 67d8673a9391..69a3a84e159e 100644
> > > > > --- a/fs/nfsd/nfs4recover.c
> > > > > +++ b/fs/nfsd/nfs4recover.c
> > > > > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > > > ci = &cmsg->cm_u.cm_clntinfo;
> > > > > if (get_user(namelen, &ci->cc_name.cn_len))
> > > > > return -EFAULT;
> > > > > + if (!namelen) {
> > > > > + dprintk("%s: namelen should not be zero", __func__);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> > >
> > > Don't you also want an upper bound sanity check?
> > > (or is cn_len only 8 bit?)
> >
> > Yeah, actually it should probably be checking for namelen >
> > NFS4_OPAQUE_LIMIT.
Scott, can you send me a tested patch? I can squash that in.
> I suspect memdup_user() itself should have a third 'maxlen' argument.
> And probably one that is required to be a compile-time constant.
>
> Oh, and is dprintk() rate-limited?
No dprintk call site is rate-limited, and they are everywhere in
this code. Since they are off by default, rate-limiting is not a
concern. Also, journald will rate-limit any output from the kernel
to prevent flooding the system lock.
> Not that the message looks very helpful.
The specific message is not helpful; suggestions welcome. But I
prefer having a warning of some kind rather than a silent failure
mode.
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 0/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall()
2024-09-09 14:10 ` Chuck Lever
@ 2024-09-09 20:28 ` Scott Mayhew
2024-09-09 20:28 ` [PATCH 1/1] " Scott Mayhew
2024-09-09 20:35 ` [PATCH 0/1] " cel
0 siblings, 2 replies; 16+ messages in thread
From: Scott Mayhew @ 2024-09-09 20:28 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs
This patch is intended to go on top of "nfsd: return -EINVAL when
namelen is 0" from Li Lingfeng. Li's patch checks for 0, but we should
be enforcing an upper bound as well.
Note that if nfsdcld somehow gets an id > NFS4_OPAQUE_LIMIT in its
database, it'll truncate it to NFS4_OPAQUE_LIMIT when it does the
downcall anyway... so to test, I had to run nfsdcld with that check
removed:
---8<---
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
index 03016fb9..fb900c7b 100644
--- a/utils/nfsdcld/sqlite.c
+++ b/utils/nfsdcld/sqlite.c
@@ -1335,8 +1335,6 @@ sqlite_iterate_recovery(int (*cb)(struct cld_client *clnt), struct cld_client *c
id = sqlite3_column_blob(stmt, 0);
id_len = sqlite3_column_bytes(stmt, 0);
- if (id_len > NFS4_OPAQUE_LIMIT)
- id_len = NFS4_OPAQUE_LIMIT;
memset(&cmsg->cm_u, 0, sizeof(cmsg->cm_u));
#if UPCALL_VERSION >= 2
---8<---
I ran the following python script to add some dummy records of varying
lengths (0, 1, 1024, 1025) to the sqlite db:
---8<---
import sqlite3
NFS4_OPAQUE_LIMIT=1024
con = sqlite3.connect("/var/lib/nfs/nfsdcld/main.sqlite")
con.row_factory = sqlite3.Row
for row in con.execute("select * from grace"):
epoch = int(row['current'])
query = 'insert into "rec-{:016x}" (id) values (?)'.format(epoch)
w = None
x = 'x'.encode()
y = ('y' * NFS4_OPAQUE_LIMIT).encode()
z = ('z' * (NFS4_OPAQUE_LIMIT + 1)).encode()
con.execute(query, (w,))
con.execute(query, (x,))
con.execute(query, (y,))
con.execute(query, (z,))
con.commit()
con.close()
---8<---
Additionally, I ensured I had a record from a valid client in the db and
that that client had a file open. I enabled NFSDDBG_PROC, restarted
nfsd, and checked for the following messages:
Sep 09 15:30:27 rhel9.smayhew.redhat.com.nfsv4.dev kernel: __cld_pipe_inprogress_downcall: invalid namelen (0)
Sep 09 15:30:27 rhel9.smayhew.redhat.com.nfsv4.dev kernel: __cld_pipe_inprogress_downcall: invalid namelen (1025)
I also verified in wireshark that my actual client was able to reclaim
its open file.
-Scott
Scott Mayhew (1):
nfsd: enforce upper limit for namelen in
__cld_pipe_inprogress_downcall()
fs/nfsd/nfs4recover.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 1/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall()
2024-09-09 20:28 ` [PATCH 0/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall() Scott Mayhew
@ 2024-09-09 20:28 ` Scott Mayhew
2024-09-09 20:35 ` [PATCH 0/1] " cel
1 sibling, 0 replies; 16+ messages in thread
From: Scott Mayhew @ 2024-09-09 20:28 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs
This patch is intended to go on top of "nfsd: return -EINVAL when
namelen is 0" from Li Lingfeng. Li's patch checks for 0, but we should
be enforcing an upper bound as well.
Note that if nfsdcld somehow gets an id > NFS4_OPAQUE_LIMIT in its
database, it'll truncate it to NFS4_OPAQUE_LIMIT when it does the
downcall anyway.
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
fs/nfsd/nfs4recover.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 69a3a84e159e..a2b995ee77f4 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -809,8 +809,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
ci = &cmsg->cm_u.cm_clntinfo;
if (get_user(namelen, &ci->cc_name.cn_len))
return -EFAULT;
- if (!namelen) {
- dprintk("%s: namelen should not be zero", __func__);
+ if (namelen == 0 || namelen > NFS4_OPAQUE_LIMIT) {
+ dprintk("%s: invalid namelen (%u)", __func__, namelen);
return -EINVAL;
}
name.data = memdup_user(&ci->cc_name.cn_id, namelen);
@@ -835,8 +835,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
cnm = &cmsg->cm_u.cm_name;
if (get_user(namelen, &cnm->cn_len))
return -EFAULT;
- if (!namelen) {
- dprintk("%s: namelen should not be zero", __func__);
+ if (namelen == 0 || namelen > NFS4_OPAQUE_LIMIT) {
+ dprintk("%s: invalid namelen (%u)", __func__, namelen);
return -EINVAL;
}
name.data = memdup_user(&cnm->cn_id, namelen);
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 0/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall()
2024-09-09 20:28 ` [PATCH 0/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall() Scott Mayhew
2024-09-09 20:28 ` [PATCH 1/1] " Scott Mayhew
@ 2024-09-09 20:35 ` cel
1 sibling, 0 replies; 16+ messages in thread
From: cel @ 2024-09-09 20:35 UTC (permalink / raw)
To: Scott Mayhew; +Cc: Chuck Lever, linux-nfs
From: Chuck Lever <chuck.lever@oracle.com>
On Mon, 09 Sep 2024 16:28:53 -0400, Scott Mayhew wrote:
> diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c
> index 03016fb9..fb900c7b 100644
>
Applied to nfsd-next for v6.12, thanks!
[1/1] nfsd: enforce upper limit for namelen in __cld_pipe_inprogress_downcall()
commit: 2760ad9b89938ce09705ab30e2087c1fb29a5bb4
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread