> On Jan 6, 2022, at 2:32 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> On Thu, Jan 6, 2022 at 12:41 PM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>>
>>
>>
>>> On Jan 5, 2022, at 3:13 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>>>
>>> On Wed, Jan 05, 2022 at 09:59:16AM -0500, rtm@xxxxxxxxxxxxx wrote:
>>>> If the special ONE stateid is passed to nfs4_preprocess_stateid_op(),
>>>> it returns status=0 but does not set *cstid. nfsd4_copy_notify()
>>>> depends on stid being set if status=0, and thus can crash if the
>>>> client sends the right COPY_NOTIFY RPC.
>>>>
>>>> I've attached a demo.
>>>>
>>>> # uname -a
>>>> Linux (none) 5.16.0-rc7-00108-g800829388818-dirty #28 SMP Wed Jan 5 14:40:37 UTC 2022 riscv64 riscv64 riscv64 GNU/Linux
>>>> # cc nfsd_5.c
>>>> # ./a.out
>>>> ...
>>>> [ 35.583265] Unable to handle kernel paging request at virtual address ffffffff00000008
>>>> [ 35.596916] status: 0000000200000121 badaddr: ffffffff00000008 cause: 000000000000000d
>>>> [ 35.597781] [<ffffffff80640cc6>] nfs4_alloc_init_cpntf_state+0x94/0xdc
>>>> [ 35.598576] [<ffffffff80274c98>] nfsd4_copy_notify+0xf8/0x28e
>>>> [ 35.599386] [<ffffffff80275c86>] nfsd4_proc_compound+0x2b6/0x4ee
>>>> [ 35.600166] [<ffffffff8025f7f4>] nfsd_dispatch+0x118/0x174
>>>> [ 35.600840] [<ffffffff8061a2e8>] svc_process_common+0x2f4/0x56c
>>>> [ 35.601630] [<ffffffff8061a624>] svc_process+0xc4/0x102
>>>> [ 35.602302] [<ffffffff8025f25a>] nfsd+0xfa/0x162
>>>> [ 35.602979] [<ffffffff80027010>] kthread+0x124/0x136
>>>> [ 35.603668] [<ffffffff8000303e>] ret_from_exception+0x0/0xc
>>>> [ 35.604667] ---[ end trace 69f12ad62072e251 ]---
>>>
>>> We could do something like this.--b.
>>>
>>> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
>>> Date: Wed Jan 5 14:15:03 2022 -0500
>>>
>>> nfsd: fix crash on COPY_NOTIFY with special stateid
>>>
>>> RTM says "If the special ONE stateid is passed to
>>> nfs4_preprocess_stateid_op(), it returns status=0 but does not set
>>> *cstid. nfsd4_copy_notify() depends on stid being set if status=0, and
>>> thus can crash if the client sends the right COPY_NOTIFY RPC."
>>>
>>> RFC 7862 says "The cna_src_stateid MUST refer to either open or locking
>>> states provided earlier by the server. If it is invalid, then the
>>> operation MUST fail."
>>>
>>> The RFC doesn't specify an error, and the choice doesn't matter much as
>>> this is clearly illegal client behavior, but bad_stateid seems
>>> reasonable.
>>>
>>> Simplest is just to guarantee that nfs4_preprocess_stateid_op, called
>>> with non-NULL cstid, errors out if it can't return a stateid.
>>>
>>> Reported-by: rtm@xxxxxxxxxxxxx
>>> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
>>> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 1956d377d1a6..b94b3bb2b8a6 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -6040,7 +6040,11 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>> *nfp = NULL;
>>>
>>> if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
>>> - status = check_special_stateids(net, fhp, stateid, flags);
>>> + if (cstid)
>>> + status = nfserr_bad_stateid;
>>> + else
>>> + status = check_special_stateids(net, fhp, stateid,
>>> + flags);
>>> goto done;
>>> }
>>
>> Thanks, Bruce. I'll take this provisionally for v5.17. Olga, can you
>> provide a Reviewed-by: ?
>
> I reproduced the original problem (thank you for the reproducer).
>
> Reviewed-by and Tested-by.
Much appreciated.
--
Chuck Lever
