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.
>
>
> --
> Chuck Lever
>
>
>
