> 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: ?
--
Chuck Lever
