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

[Chuck Lever <chuck.lever@oracle.com>]

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?


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


> +}
> +
> +/**
> + * 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 ?


> +
> +	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?


> +
> +	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