Re: [PATCH] nfsd: return -EINVAL when namelen is 0

[Li Lingfeng <lilingfeng3@huawei.com>]

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