On 3/17/25 1:48 PM, Olga Kornievskaia wrote:
> 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)
<shudder>
I guess we're stuck with this because this function is used for both
NFS and NLM file handles.
Let's use a symbolic constant here, at least.
> + 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
>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>>
--
Chuck Lever
