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