On Thu, 13 Mar 2025, Olga Kornievskaia 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.
Can you see if this fixes it?
Maybe we need to bypass tls as well as gss
Thanks,
NeilBrown
--- 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: */
