Hi Olga -
Thanks for taking a stab at this. Comments below.
On 3/19/25 5:46 PM, Olga Kornievskaia wrote:
> Since commit 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK")
> for export policies with "sec=krb5:..." or "xprtsec=tls:.." NLM
> locking calls on v3 mounts fail. And for "sec=krb5" NLM calls it
> also leads to out-of-bounds reference while in check_nfsd_access().
>
> This patch does 3 fixes.
That suggests to me this should ideally be three patches. That's a nit,
really, but 3 patches might be better for subsequent bisection.
> 2 problems are related to sec=krb5.
> First is fixing what "access" content is being passed into
> the inode_permission(). Prior to 4cc9b9f2bf4df, the code would
> explicitly set access to be read/ownership. And after is passes
> access that's set in nlm_fopen but it's lacking read access.
> Second is because previously for NLM check_nfsd_access() was
> never called and thus nfsd4_spo_must_allow() function wasn't
> called. After the patch, this lead to NLM call which has no
> compound state structure created trying to dereference it.
> This patch instead moves the call to after may_bypass_gss
> check which implies NLM and would return there and would
> never get to calling nfsd4_spo_must_allow().
>
> The last problem is related to TLS export policy. NLM dont
> come over TLS and thus dont pass the TLS checks in
> check_nfsd_access() leading to access being denied. Instead
> rely on may_bypass_gss to indicate NLM and allow access
> checking to continue.
NFSD doesn't check that NLM is TLS protected because:
a. the current Linux NFS client doesn't use TLS, and
b. NFSD doesn't support NLM over krb5, IIRC.
But that doesn't mean NLM will /never/ be protected by TLS.
I'm thinking aloud about whether the reorganization here might make
adding TLS for NLM easier or more difficult. No conclusions yet.
> Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK")
> Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
>
> ---
> fs/nfsd/export.c | 23 +++++++++++++----------
> fs/nfsd/vfs.c | 4 ++++
> 2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 0363720280d4..4cbf617efa7c 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1124,7 +1124,8 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> goto ok;
> }
> - goto denied;
> + if (!may_bypass_gss)
> + goto denied;
>
> ok:
> /* legacy gss-only clients are always OK: */
> @@ -1142,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 +1160,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> }
> }
>
> + /* 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
> + */
> + /* This call must be done after the may_bypass_gss check.
> + * may_bypass_gss implies NLM(/MOUNT) and not 4.1
> + */
> + if (nfsd4_spo_must_allow(rqstp))
> + return nfs_ok;
> +
Comment about future work: This is technical debt (that's the 21st
century term for spaghetti), logic that has accrued over time and is
now therefore difficult to reason about. Would be nice to break
check_nfsd_access() apart into "for NLM", "for NFS-legacy", and "for NFS
with COMPOUND". For another day, perhaps.
> denied:
> return nfserr_wrongsec;
> }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 4021b047eb18..95d973136079 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -2600,6 +2600,10 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
More context shows there is an NFSD_MAY_OWNER_OVERRIDE check just
before the new logic added by this patch:
if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
> uid_eq(inode->i_uid, current_fsuid()))
> return 0;
>
> + /* If this is NLM, require read or ownership permissions */
> + if (acc & NFSD_MAY_NLM)
> + acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> +
So I'm wondering if this new test needs to come /before/ the existing
MAY_OWNER_OVERRIDE check here? If not, the comment you add here might
explain why it is correct to place the new logic afterwards.
> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> err = inode_permission(&nop_mnt_idmap, inode,
> acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
--
Chuck Lever
