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 ]---
On Wed, Jan 05, 2022 at 09:59:16AM -0500, [email protected] 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 <[email protected]>
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: [email protected]
Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
Signed-off-by: J. Bruce Fields <[email protected]>
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;
}
> On Jan 5, 2022, at 3:13 PM, J. Bruce Fields <[email protected]> wrote:
>
> On Wed, Jan 05, 2022 at 09:59:16AM -0500, [email protected] 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 <[email protected]>
> 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: [email protected]
> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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
On Thu, Jan 6, 2022 at 12:41 PM Chuck Lever III <[email protected]> wrote:
>
>
>
> > On Jan 5, 2022, at 3:13 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > On Wed, Jan 05, 2022 at 09:59:16AM -0500, [email protected] 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 <[email protected]>
> > 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: [email protected]
> > Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> > Signed-off-by: J. Bruce Fields <[email protected]>
> >
> > 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
>
>
>
> On Jan 6, 2022, at 2:32 PM, Olga Kornievskaia <[email protected]> wrote:
>
> On Thu, Jan 6, 2022 at 12:41 PM Chuck Lever III <[email protected]> wrote:
>>
>>
>>
>>> On Jan 5, 2022, at 3:13 PM, J. Bruce Fields <[email protected]> wrote:
>>>
>>> On Wed, Jan 05, 2022 at 09:59:16AM -0500, [email protected] 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 <[email protected]>
>>> 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: [email protected]
>>> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
>>> Signed-off-by: J. Bruce Fields <[email protected]>
>>>
>>> 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