Re: [PATCH 2/3] nfsd: adjust nfsd4_spo_must_allow checking order

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2025-04-07 at 13:17 -0400, Olga Kornievskaia wrote:
> On Mon, Apr 7, 2025 at 11:59 AM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2025-04-07 at 11:56 -0400, Olga Kornievskaia wrote:
> > > On Mon, Apr 7, 2025 at 11:36 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > On Fri, 2025-03-21 at 20:13 -0400, Olga Kornievskaia wrote:
> > > > > Prior to this patch, some non-4.x NFS operations such as NLM
> > > > > calls have to go thru export policy checking would end up
> > > > > calling nfsd4_spo_must_allow() function and lead to an
> > > > > out-of-bounds error because no compound state structures
> > > > > needed by nfsd4_spo_must_allow() are present in the svc_rqst
> > > > > request structure.
> > > > > 
> > > > > Instead, do the nfsd4_spo_must_allow() checking after the
> > > > > may_bypass_gss check which is geared towards allowing various
> > > > > calls such as NLM while export policy is set with sec=krb5:...
> > > > > 
> > > > > Fixes: 4cc9b9f2bf4d ("nfsd: refine and rename NFSD_MAY_LOCK")
> > > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > > > > ---
> > > > >  fs/nfsd/export.c | 17 ++++++++---------
> > > > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > > > index 88ae410b4113..02f26cbd59d0 100644
> > > > > --- a/fs/nfsd/export.c
> > > > > +++ b/fs/nfsd/export.c
> > > > > @@ -1143,15 +1143,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > > > >                       return nfs_ok;
> > > > >       }
> > > > > 
> > > > > -     /* If the compound op contains a spo_must_allowed op,
> > > > > -      * it will be sent with integrity/protection which
> > > > > -      * will have to be expressly allowed on mounts that
> > > > > -      * don't support it
> > > > > -      */
> > > > > -
> > > > > -     if (nfsd4_spo_must_allow(rqstp))
> > > > > -             return nfs_ok;
> > > > > -
> > > > >       /* Some calls may be processed without authentication
> > > > >        * on GSS exports. For example NFS2/3 calls on root
> > > > >        * directory, see section 2.3.2 of rfc 2623.
> > > > > @@ -1168,6 +1159,14 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > > > >                               return 0;
> > > > >               }
> > > > >       }
> > > > > +     /* If the compound op contains a spo_must_allowed op,
> > > > > +      * it will be sent with integrity/protection which
> > > > > +      * will have to be expressly allowed on mounts that
> > > > > +      * don't support it
> > > > > +      */
> > > > > +     if (nfsd4_spo_must_allow(rqstp))
> > > > > +             return nfs_ok;
> > > > > +
> > > > > 
> > > > >  denied:
> > > > >       return nfserr_wrongsec;
> > > > 
> > > > Is this enough to fully fix the OOB problem? It looks like you could
> > > > still get past the may_bypass_gss if statement above this with a
> > > > carefully crafted RPC.
> > > 
> > > A crafted RPC can and thus Neil's patch that checks the version in
> > > nfsd4_spo_must_allow is needed.
> > > 
> > > I still feel changing the order would be beneficial as it would take
> > > care of realistic requests.
> > 
> > No objection to changing the order if that makes sense, but I think we
> > do need to guard against carefully crafted RPCs too. Can we have
> > nfsd4_spo_must_allow() vet that the request is NFSv4 before checking
> > the compound fields too?
> 
> Neil already posted a patch for that? "nfsd: nfsd4_spo_must_allow()
> must check this is a v4 compound request" march 27th.
> 
> 

Perfect. You guys are way ahead of me!

With that in place, what's the benefit to taking this patch? Does
reordering these checks give us anything?
-- 
Jeff Layton <jlayton@kernel.org>