Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew <smayhew@redhat.com>]

On Fri, 01 Aug 2025, Chuck Lever wrote:

> On 7/31/25 5:14 PM, Scott Mayhew wrote:
> > A while back I had reported that an NFSv3 client could successfully
> > mount using '-o xprtsec=none' an export that had been exported with
> > 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> > would succeed and the mount would show up in /proc/mounts.  Attempting
> > to do anything futher with the mount would be met with NFS3ERR_ACCES.
> 
> I agree, though it doesn't compromise access to file data, that's not
> the most desirable behavior.
> 
> Can you find your report on lore, and a Link: to it here in the patch
> description?

Will do.
> 
> 
> > This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> > NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> > subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> > NLM under XPRTSEC policies").
> > 
> > Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> > so they shouldn't be conflated when determining whether the access
> > checks can be bypassed.
> 
> Fixes: 9280c5774314 ("NFSD: Handle new xprtsec= export option")
> 
> 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
> >  fs/nfsd/export.h   |  1 +
> >  fs/nfsd/nfs4proc.c |  6 ++++-
> >  fs/nfsd/nfs4xdr.c  |  3 +++
> >  fs/nfsd/nfsfh.c    |  8 +++++++
> >  fs/nfsd/vfs.c      |  3 +++
> >  6 files changed, 67 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index cadfc2bae60e..bc54b01bb516 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> >  }
> >  
> >  /**
> > - * check_nfsd_access - check if access to export is allowed.
> > + * check_xprtsec_policy - check if access to export is permitted by the
> > + * 			  xprtsec policy
> >   * @exp: svc_export that is being accessed.
> >   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> > - * @may_bypass_gss: reduce strictness of authorization check
> > + *
> > + * This logic should not be combined with check_nfsd_access, as the rules
> > + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> > + * check:
> > + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> > + * 	  but that doesn't mean they can bypass the xprtsec poolicy check
> > + * 	- NLM can bypass the GSS check on exports exported with the
> > + * 	  NFSEXP_NOAUTHNLM flag
> > + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> > + * 	  implemented for the sidecar protocols
> >   *
> >   * Return values:
> >   *   %nfs_ok if access is granted, or
> > - *   %nfserr_wrongsec if access is denied
> > + *   %nfserr_acces or %nfserr_wrongsec if access is denied
> >   */
> > -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > -			 bool may_bypass_gss)
> > +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
> >  {
> > -	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> >  	struct svc_xprt *xprt;
> >  
> >  	/*
> > @@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
> >  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
> >  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
> >  		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
> >  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
> >  		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> > -	if (!may_bypass_gss)
> > -		goto denied;
> >  
> > -ok:
> > +	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
> 
> We recently spilled a lot of electrons trying to get these version
> checks out of the generic security checking paths. For one thing, this
> particular check is valid only for the NFS program.
> 
> Returning nfserr_wrongsec unconditionally, as check_nfsd_access now
> does, should be sufficient.

Okay.
> 
> 
> > +}
> > +
> > +/**
> > + * check_nfsd_access - check if access to export is allowed.
> > + * @exp: svc_export that is being accessed.
> > + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> > + * @may_bypass_gss: reduce strictness of authorization check
> > + *
> > + * Return values:
> > + *   %nfs_ok if access is granted, or
> > + *   %nfserr_wrongsec if access is denied
> > + */
> > +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > +			 bool may_bypass_gss)
> > +{
> > +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > +	struct svc_xprt *xprt;
> > +
> > +	/*
> > +	 * If rqstp is NULL, this is a LOCALIO request which will only
> > +	 * ever use a filehandle/credential pair for which access has
> > +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> > +	 * wire. So there is no need for further checks here.
> > +	 */
> > +	if (!rqstp)
> > +		return nfs_ok;
> 
> Is this true of all of check_nfsd_access's callers, or only of
> __fh_verify ?
> 
Looking at the commit where this check was added, and looking at the
other callers, it looks like this is only true of __fh_verify().

I'm splitting up check_nfsd_access() into two helpers has you suggested,
having __fh_verify() call the helpers directly while having the other
callers continue to use check_nfsd_access().

Should I add an argument to the helpers indicate when they have been
called directly?  Something like 'bool maybe_localio', which can
then be incorporated into the above check, e.g.

        if (!rqstp) {
                if (maybe_localio) {
                        return nfs_ok;
                } else {
                        WARN_ON_ONCE(1);
                        return nfserr_wrongsec;
                }
        }



> 
> > +
> > +	xprt = rqstp->rq_xprt;
> > +
> >  	/* legacy gss-only clients are always OK: */
> >  	if (exp->ex_client == rqstp->rq_gssclient)
> >  		return nfs_ok;
> > @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  		}
> >  	}
> >  
> > -denied:
> >  	return nfserr_wrongsec;
> >  }
> >  
> > diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> > index b9c0adb3ce09..c5a45f4b72be 100644
> > --- a/fs/nfsd/export.h
> > +++ b/fs/nfsd/export.h
> > @@ -101,6 +101,7 @@ struct svc_expkey {
> >  
> >  struct svc_cred;
> >  int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
> > +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
> >  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  			 bool may_bypass_gss);
> >  
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 71b428efcbb5..71e9a170f7bf 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
> >  				clear_current_stateid(cstate);
> >  
> >  			if (current_fh->fh_export &&
> > -					need_wrongsec_check(rqstp))
> > +					need_wrongsec_check(rqstp)) {
> > +				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
> > +				if (op->status)
> > +					goto encode_op;
> >  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> > +			}
> >  		}
> >  encode_op:
> >  		if (op->status == nfserr_replay_me) {
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index ea91bad4eee2..48d55c13c918 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
> >  			nfserr = nfserrno(err);
> >  			goto out_put;
> >  		}
> > +		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
> > +		if (nfserr)
> > +			goto out_put;
> >  		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
> >  		if (nfserr)
> >  			goto out_put;
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 74cf1f4de174..1ffc33662582 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
> >  	if (error)
> >  		goto out;
> >  
> > +	if (access & NFSD_MAY_NLM)
> > +		/* NLM is allowed to bypass the xprtssec policy check */
> 		/* because lockd currently does not support xprtsec */> +		goto out;
> Every check_xprtsec_policy / check_nfsd_access call site now has two
> function calls, resulting in duplicate code.
> 
> Why not leave check_nfsd_access() in place, but replace it's guts with
> two helpers, and then call the two helpers directly here in __fh_verify?

I'll create the two helpers as you suggest.

I'll still need to check the access flags for NFSD_MAY_NLM before
calling the xprtsec helper though (I'll make sure I do it in the right
place this time).

-Scott
> 
> 
> > +
> > +	error = check_xprtsec_policy(exp, rqstp);
> > +	if (error)
> > +		goto out;
> > +
> >  	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
> >  		/* NLM is allowed to fully bypass authentication */
> >  		goto out;
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 98ab55ba3ced..1b66aff1d750 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> >  	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
> >  	if (err)
> >  		return err;
> > +	err = check_xprtsec_policy(exp, rqstp);
> > +	if (err)
> > +		goto out;
> >  	err = check_nfsd_access(exp, rqstp, false);
> >  	if (err)
> >  		goto out;
> 
> 
> -- 
> Chuck Lever
>