On Wed, Mar 12, 2025 at 8:09 PM NeilBrown <neilb@xxxxxxx> wrote:
>
> On Thu, 13 Mar 2025, Olga Kornievskaia wrote:
> > On Wed, Mar 12, 2025 at 9:22 AM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> > >
> > > On Tue, Mar 11, 2025 at 5:42 PM NeilBrown <neilb@xxxxxxx> wrote:
> > > >
> > > > On Wed, 12 Mar 2025, Olga Kornievskaia wrote:
> > > > > On Tue, Mar 11, 2025 at 11:28 AM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Thu, Oct 17, 2024 at 5:42 PM NeilBrown <neilb@xxxxxxx> wrote:
> > > > > > >
> > > > > > >
> > > > > > > NFSD_MAY_LOCK means a few different things.
> > > > > > > - it means that GSS is not required.
> > > > > > > - it means that with NFSEXP_NOAUTHNLM, authentication is not required
> > > > > > > - it means that OWNER_OVERRIDE is allowed.
> > > > > > >
> > > > > > > None of these are specific to locking, they are specific to the NLM
> > > > > > > protocol.
> > > > > > > So:
> > > > > > > - rename to NFSD_MAY_NLM
> > > > > > > - set NFSD_MAY_OWNER_OVERRIDE and NFSD_MAY_BYPASS_GSS in nlm_fopen()
> > > > > > > so that NFSD_MAY_NLM doesn't need to imply these.
> > > > > > > - move the test on NFSEXP_NOAUTHNLM out of nfsd_permission() and
> > > > > > > into fh_verify where other special-case tests on the MAY flags
> > > > > > > happen. nfsd_permission() can be called from other places than
> > > > > > > fh_verify(), but none of these will have NFSD_MAY_NLM.
> > > > > >
> > > > > > This patch breaks NLM when used in combination with TLS.
> > > > >
> > > > > I was too quick to link this to TLS. It's presence of security policy
> > > > > so sec=krb* causes the same problems.
> > > > >
> > > > > > If exports
> > > > > > have xprtsec=tls:mtls and mount is done with tls/mtls, the server
> > > > > > won't give any locks and client will get "no locks available" error.
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > > > > > > ---
> > > > > > >
> > > > > > > No change from previous patch - the corruption in the email has been
> > > > > > > avoided (I hope).
> > > > > > >
> > > > > > >
> > > > > > > fs/nfsd/lockd.c | 13 +++++++++++--
> > > > > > > fs/nfsd/nfsfh.c | 12 ++++--------
> > > > > > > fs/nfsd/trace.h | 2 +-
> > > > > > > fs/nfsd/vfs.c | 12 +-----------
> > > > > > > fs/nfsd/vfs.h | 2 +-
> > > > > > > 5 files changed, 18 insertions(+), 23 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > > > > > > index 46a7f9b813e5..edc9f75dc75c 100644
> > > > > > > --- a/fs/nfsd/lockd.c
> > > > > > > +++ b/fs/nfsd/lockd.c
> > > > > > > @@ -38,11 +38,20 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> > > > > > > memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
> > > > > > > fh.fh_export = NULL;
> > > > > > >
> > > > > > > + /*
> > > > > > > + * Allow BYPASS_GSS as some client implementations use AUTH_SYS
> > > > > > > + * for NLM even when GSS is used for NFS.
> > > > > > > + * Allow OWNER_OVERRIDE as permission might have been changed
> > > > > > > + * after the file was opened.
> > > > > > > + * Pass MAY_NLM so that authentication can be completely bypassed
> > > > > > > + * if NFSEXP_NOAUTHNLM is set. Some older clients use AUTH_NULL
> > > > > > > + * for NLM requests.
> > > > > > > + */
> > > > > > > access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
> > > > > > > - access |= NFSD_MAY_LOCK;
> > > > > > > + access |= NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | NFSD_MAY_BYPASS_GSS;
> > > > > > > nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
> > > > > > > fh_put(&fh);
> > > > > > > - /* We return nlm error codes as nlm doesn't know
> > > > > > > + /* We return nlm error codes as nlm doesn't know
> > > > > > > * about nfsd, but nfsd does know about nlm..
> > > > > > > */
> > > > > > > switch (nfserr) {
> > > > > > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > > > > > > index 40533f7c7297..6a831cb242df 100644
> > > > > > > --- a/fs/nfsd/nfsfh.c
> > > > > > > +++ b/fs/nfsd/nfsfh.c
> > > > > > > @@ -363,13 +363,10 @@ __fh_verify(struct svc_rqst *rqstp,
> > > > > > > if (error)
> > > > > > > goto out;
> > > > > > >
> > > > > > > - /*
> > > > > > > - * pseudoflavor restrictions are not enforced on NLM,
> > > > > > > - * which clients virtually always use auth_sys for,
> > > > > > > - * even while using RPCSEC_GSS for NFS.
> > > > > > > - */
> > > > > > > - if (access & NFSD_MAY_LOCK)
> > > > > > > - goto skip_pseudoflavor_check;
> > > > > > > + if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
> > > > >
> > > > > I think this should either be an OR or the fact that "nlm but only
> > > > > with insecurelock export option and not other" is the only way to
> > > > > bypass checking is wrong. I think it's just a check for NLM that
> > > > > stays.
> > > >
> > > > I don't think that NLM gets a complete bypass unless no_auth_nlm is set.
> > > > For the case you are describing, I think NFSD_MAY_BYPASS_GSS is supposed
> > > > to make it work.
> > > >
> > > > I assume the NLM request is arriving with AUTH_SYS authentication?
> > >
> > > It does.
> > >
> > > Just to give you a practical example. exports have
> > > (rw,...,sec=krb5:krb5i:krb5p). Client does mount with sec=krb5. Then
> > > does an flock() on the file. What's more I have just now hit Kasan's
> > > out-of-bounds warning on that. I'll have to see if that exists on 6.14
> > > (as I'm debugging the matter on the commit of the patch itself and
> > > thus on 6.12-rc now).
> > >
> > > I will layout more reasoning but what allowed NLM to work was this
> > > - /*
> > > - * pseudoflavor restrictions are not enforced on NLM,
> > > - * which clients virtually always use auth_sys for,
> > > - * even while using RPCSEC_GSS for NFS.
> > > - */
> > > - if (access & NFSD_MAY_LOCK)
> > > - goto skip_pseudoflavor_check;
> > >
> > > but I don't know why the replacement doesn't work.
> >
> > As I mentioned the patch removed the skip_pseudoflavor check (that for
> > NLM) would have bypassed calling check_nfsd_access(). Instead, the
> > problem is that even though may_bypass_gss is set to true it call into
> > nfsd4_spo_must_allow(rqstp) which now wrongly assumes there is
> > compound state (struct nfsd4_compound_state *cstate = &resp->cstate;)
> > ... (but this is NLM). So it proceed to deference it if
> > (!cstate->minorversion) causing the KASAN to do the out-of-bound error
> > that I mentioned. It most of the time now cause a crash. But on the
> > off non-deterministic times when it completes it fails.
> >
> > I really don't think calling into check_nfsd_access() is appropriate for NLM.
>
> Why not? What is there is inherently inappropriate for NLM?
I spoke too soon. It's not about calling into check_nfsd_acces()
(though it's a problem for v3 because there isn't a compound state!).
The real problem is "access" content that's being passed into the
nfsd_permission() function.
I don't fully understand the logic. But before this patch, "access"
(acc) was (re)set to "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE" before
calling into inode_permission():
@@ -2534,16 +2534,6 @@ nfsd_permission(struct svc_cred *cred, struct
svc_export *exp,
if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
return nfserr_perm;
- if (acc & NFSD_MAY_LOCK) {
- /* If we cannot rely on authentication in NLM requests,
- * just allow locks, otherwise require read permission, or
- * ownership
- */
- if (exp->ex_flags & NFSEXP_NOAUTHNLM)
- return 0;
- else
- acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
- }
/*
* The file owner always gets access permission for accesses that
* would normally be checked at open time. This is to make
now it's doesn't get "reset" and passes all of what was set in nlm_fopen()
access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
- access |= NFSD_MAY_LOCK;
+ access |= NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | NFSD_MAY_BYPASS_GSS;
which ends up being "write" instead of a read and inode_permission returned -13.
Here's my proposed fix for one the problems in the patch.
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 4021b047eb18..eb139962ac4c 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2600,6 +2600,9 @@ nfsd_permission(struct svc_cred *cred, struct
svc_export *exp,
uid_eq(inode->i_uid, current_fsuid()))
return 0;
+ if (acc & NFSD_MAY_NLM)
+ acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
+
/* 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));
Now the 2nd problem. You mentioned checking for version before calling
nfsd4_spo_must_allow for v3. So something like this perhaps but not
sure if that's right?
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 0363720280d4..0106da76da89 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1148,8 +1148,9 @@ __be32 check_nfsd_access(struct svc_export *exp,
struct svc_rqst *rqstp,
* don't support it
*/
- if (nfsd4_spo_must_allow(rqstp))
- return nfs_ok;
+ if (rqstp->rq_prog == 100003)
+ 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
In the end, I question, why not revert the original patch instead?
> I agree that calling nfsd4_spo_must_allow(rqstp) isn't appropriate for
> NLM, but it isn't appropriate for v2 or v3 either. We should add
> version checks either before it is called or before the minorversion
> check that it already has.
>
> NeilBrown
>
>
> >
> > >
> > > > So check_nfsd_access() is being called with may_bypass_gss and this:
> > > >
> > > > if (may_bypass_gss && (
> > > > rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
> > > > rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)) {
> > > > for (f = exp->ex_flavors; f < end; f++) {
> > > > if (f->pseudoflavor >= RPC_AUTH_DES)
> > > > return 0;
> > > > }
> > > > }
> > > >
> > > > in check_nfsd_access() should succeed.
> > > > Can you add some tracing and see what is happening in here?
> > > > Maybe the "goto denied" earlier in the function is being reached. I
> > > > don't fully understand the TLS code yet - maybe it needs some test on
> > > > may_bypass_gss.
> > > >
> > > > Thanks,
> > > > NeilBrown
> > > >
> > > >
> > > > >
> > > > > > > + /* NLM is allowed to fully bypass authentication */
> > > > > > > + goto out;
> > > > > > > +
> > > > > > > if (access & NFSD_MAY_BYPASS_GSS)
> > > > > > > may_bypass_gss = true;
> > > > > > > /*
> > > > > > > @@ -385,7 +382,6 @@ __fh_verify(struct svc_rqst *rqstp,
> > > > > > > if (error)
> > > > > > > goto out;
> > > > > > >
> > > > > > > -skip_pseudoflavor_check:
> > > > > > > /* Finally, check access permissions. */
> > > > > > > error = nfsd_permission(cred, exp, dentry, access);
> > > > > > > out:
> > > > > > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > > > > > > index b8470d4cbe99..3448e444d410 100644
> > > > > > > --- a/fs/nfsd/trace.h
> > > > > > > +++ b/fs/nfsd/trace.h
> > > > > > > @@ -79,7 +79,7 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode);
> > > > > > > { NFSD_MAY_READ, "READ" }, \
> > > > > > > { NFSD_MAY_SATTR, "SATTR" }, \
> > > > > > > { NFSD_MAY_TRUNC, "TRUNC" }, \
> > > > > > > - { NFSD_MAY_LOCK, "LOCK" }, \
> > > > > > > + { NFSD_MAY_NLM, "NLM" }, \
> > > > > > > { NFSD_MAY_OWNER_OVERRIDE, "OWNER_OVERRIDE" }, \
> > > > > > > { NFSD_MAY_LOCAL_ACCESS, "LOCAL_ACCESS" }, \
> > > > > > > { NFSD_MAY_BYPASS_GSS_ON_ROOT, "BYPASS_GSS_ON_ROOT" }, \
> > > > > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > > > > > index 51f5a0b181f9..2610638f4301 100644
> > > > > > > --- a/fs/nfsd/vfs.c
> > > > > > > +++ b/fs/nfsd/vfs.c
> > > > > > > @@ -2509,7 +2509,7 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
> > > > > > > (acc & NFSD_MAY_EXEC)? " exec" : "",
> > > > > > > (acc & NFSD_MAY_SATTR)? " sattr" : "",
> > > > > > > (acc & NFSD_MAY_TRUNC)? " trunc" : "",
> > > > > > > - (acc & NFSD_MAY_LOCK)? " lock" : "",
> > > > > > > + (acc & NFSD_MAY_NLM)? " nlm" : "",
> > > > > > > (acc & NFSD_MAY_OWNER_OVERRIDE)? " owneroverride" : "",
> > > > > > > inode->i_mode,
> > > > > > > IS_IMMUTABLE(inode)? " immut" : "",
> > > > > > > @@ -2534,16 +2534,6 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
> > > > > > > if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
> > > > > > > return nfserr_perm;
> > > > > > >
> > > > > > > - if (acc & NFSD_MAY_LOCK) {
> > > > > > > - /* If we cannot rely on authentication in NLM requests,
> > > > > > > - * just allow locks, otherwise require read permission, or
> > > > > > > - * ownership
> > > > > > > - */
> > > > > > > - if (exp->ex_flags & NFSEXP_NOAUTHNLM)
> > > > > > > - return 0;
> > > > > > > - else
> > > > > > > - acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> > > > > > > - }
> > > > > > > /*
> > > > > > > * The file owner always gets access permission for accesses that
> > > > > > > * would normally be checked at open time. This is to make
> > > > > > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > > > > > > index 854fb95dfdca..f9b09b842856 100644
> > > > > > > --- a/fs/nfsd/vfs.h
> > > > > > > +++ b/fs/nfsd/vfs.h
> > > > > > > @@ -20,7 +20,7 @@
> > > > > > > #define NFSD_MAY_READ 0x004 /* == MAY_READ */
> > > > > > > #define NFSD_MAY_SATTR 0x008
> > > > > > > #define NFSD_MAY_TRUNC 0x010
> > > > > > > -#define NFSD_MAY_LOCK 0x020
> > > > > > > +#define NFSD_MAY_NLM 0x020 /* request is from lockd */
> > > > > > > #define NFSD_MAY_MASK 0x03f
> > > > > > >
> > > > > > > /* extra hints to permission and open routines: */
> > > > > > >
> > > > > > > base-commit: c4e418a53fe30d8e1da68f5aabca352b682fd331
> > > > > > > --
> > > > > > > 2.46.0
> > > > > > >
> > > > > > >
> > > > >
> >
>
