On Thu, 2025-07-31 at 17:14 -0400, 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.
>
> 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.
>
> Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> ---
> 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
nit: "policy"
> + * - 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;
> +}
> +
> +/**
> + * 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;
> +
> + 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 */
> + goto out;
> +
> + 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;
The rest looks fine to me (though I wouldn't object to adding a helper
that calls both functions, like Neil suggested).
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
