MIME-Version: 1.0
References: <7318.1641394756@crash.local> <20220105201313.GE25384@fieldses.org>
 <88B4B5A4-74B1-4D6C-8210-BE6C754EE5FF@oracle.com>
In-Reply-To: <88B4B5A4-74B1-4D6C-8210-BE6C754EE5FF@oracle.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Thu, 6 Jan 2022 14:32:31 -0500
Message-ID: <CAN-5tyGAu-LfcaB0V1QvNS533PmqEP7Zqgxd=oQEaxFGmCn+_A@mail.gmail.com>
Subject: Re: nfsd v4 server can crash in COPY_NOTIFY
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Bruce Fields <bfields@fieldses.org>,
        Olga Kornievskaia <kolga@netapp.com>,
        "rtm@csail.mit.edu" <rtm@csail.mit.edu>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 6, 2022 at 12:41 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 5, 2022, at 3:13 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Wed, Jan 05, 2022 at 09:59:16AM -0500, rtm@csail.mit.edu 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@redhat.com>
> > 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@csail.mit.edu
> >    Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> >    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> >
> > 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
>
>
>