| Message ID | 20250319214641.27699-1-okorniev@redhat.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | [1/1] nfsd: fix NLM access checking | expand |
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@redhat.com> > > --- > 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));
On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > 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. I can break it into 3 patches but all would have "Fixes" for the same patch, right? > > 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. But if the future NFSD would support NLM with TLS wouldn't it be a new feature with possible new controls. We'd still need existing code to all interoperability with clients that don't support it (and thus having a configuration that would allow NLM without TLS when xprtsec is TLS-enabled? > I'm thinking aloud about whether the reorganization here might make > adding TLS for NLM easier or more difficult. No conclusions yet. Do we need that complexity now vs if and when such support is added? > > Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK") > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > > > --- > > 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. Why would you think this check should go before? NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it arrives in check_nfsd_access() . Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't NLM asking for write permissions on the file for locking? My attempt was to return the code to previous functionality which worked (and was only explicitly asking for read permissions on the file). Unless you think more is needed here: I would resubmit with 3 patches for each of the chunks and corresponding problems. > > /* 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 >
On 3/20/25 12:29 PM, Olga Kornievskaia wrote: > On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> 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. > > I can break it into 3 patches but all would have "Fixes" for the same > patch, right? Yes. >>> 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. > > But if the future NFSD would support NLM with TLS wouldn't it be a new > feature with possible new controls. We'd still need existing code to > all interoperability with clients that don't support it (and thus > having a configuration that would allow NLM without TLS when xprtsec > is TLS-enabled? > >> I'm thinking aloud about whether the reorganization here might make >> adding TLS for NLM easier or more difficult. No conclusions yet. > > Do we need that complexity now vs if and when such support is added? The question is would we need to revert some or all of your patch to make that work in the future in order to support NLM with TLS. But perhaps that is an unanswerable question today. >>> Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK") >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>> >>> --- >>> 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. > > Why would you think this check should go before? Because this code is confusing. So, for NLM, MAY_OWNER_OVERRIDE is already set for the uid_eq check. The order of the new code is not consequential. But it still catches the eye. > NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it > arrives in check_nfsd_access() . This code is /clearing/ the other flags too. That's a little subtle. The new comment should explain why only these two flags are needed for the subsequent code. That is, explain not only why NFSD_MAY_READ is getting set, but also why NFSD_MAY_NLM and NFSD_MAY_BYPASS_GSS are getting cleared. Or, if clearing those flags was unintentional, make the code read: acc |= NFSD_MAY_READ; I don't understand this code well enough to figure out why nlm_fopen() shouldn't just set NFSD_MAY_READ. Therefore: better code comment needed. > Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ > | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets > NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | > NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't > NLM asking for write permissions on the file for locking? > > My attempt was to return the code to previous functionality which > worked (and was only explicitly asking for read permissions on the > file). So the patch is reverting part of 4cc9b9f2bf4df; perhaps that should be called out in the patch description (up to you). However, the proposed fix doesn't make this code easier to understand, and that's probably my main quibble. > Unless you think more is needed here: I would resubmit with 3 patches > for each of the chunks and corresponding problems. Agreed, and I don't think I have any substantial change requests for the first two fixes. >>> /* 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 >>
On Thu, Mar 20, 2025 at 1:32 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 3/20/25 12:29 PM, Olga Kornievskaia wrote: > > On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@oracle.com> wrote: > >> > >> 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. > > > > I can break it into 3 patches but all would have "Fixes" for the same > > patch, right? > > Yes. > > > >>> 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. > > > > But if the future NFSD would support NLM with TLS wouldn't it be a new > > feature with possible new controls. We'd still need existing code to > > all interoperability with clients that don't support it (and thus > > having a configuration that would allow NLM without TLS when xprtsec > > is TLS-enabled? > > > >> I'm thinking aloud about whether the reorganization here might make > >> adding TLS for NLM easier or more difficult. No conclusions yet. > > > > Do we need that complexity now vs if and when such support is added? > > The question is would we need to revert some or all of your patch > to make that work in the future in order to support NLM with TLS. > But perhaps that is an unanswerable question today. > > > >>> Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK") > >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > >>> > >>> --- > >>> 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. > > > > Why would you think this check should go before? > > Because this code is confusing. > > So, for NLM, MAY_OWNER_OVERRIDE is already set for the uid_eq check. > The order of the new code is not consequential. But it still catches > the eye. I just re-checked the original code and the re(set)ing of acc was done before the MAY_OWNER_OVERRIDE check. So changing my patch as you suggest would be consistent with how things worked before. I will do that. > > NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it > > arrives in check_nfsd_access() . > > This code is /clearing/ the other flags too. That's a little subtle. > > The new comment should explain why only these two flags are needed for > the subsequent code. inode_permission() only care about READ, WRITE (EXEC)? NFSD_MAY_READ is supposed to be the same as MAY_READ etc. Thus I believe passing other values is of no consequence so to be honest I don't understand why NFSD_MAY_OWNER_OVERRIDE is needed here. But what is of consequence is (not) passing a NFSD_MAY_WRITE (which is what gets passed in by nlm_fopen()). > That is, explain not only why NFSD_MAY_READ is > getting set, but also why NFSD_MAY_NLM and NFSD_MAY_BYPASS_GSS are > getting cleared. > > Or, if clearing those flags was unintentional, make the code read: > > acc |= NFSD_MAY_READ; > > I don't understand this code well enough to figure out why nlm_fopen() > shouldn't just set NFSD_MAY_READ. Therefore: better code comment needed. My comment came from the comment that was removed by commit 4cc9b9f2bf4df. I don't have a good understanding of the code to provide a "better" comment. nlm_fopen assigns the access based on the requested lock type. an exclusive lock i'm assuming gets translated to the "write" = NFSD_MAY_WRITE. But the the user might not have write access to the file but still be allowed to request an exclusive lock on it, right? @@ -2531,16 +2531,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 > > > > Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ > > | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets > > NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | > > NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't > > NLM asking for write permissions on the file for locking? > > > > My attempt was to return the code to previous functionality which > > worked (and was only explicitly asking for read permissions on the > > file). > > So the patch is reverting part of 4cc9b9f2bf4df; perhaps that should be > called out in the patch description (up to you). However, the proposed > fix doesn't make this code easier to understand, and that's probably my > main quibble. > > > > Unless you think more is needed here: I would resubmit with 3 patches > > for each of the chunks and corresponding problems. > > Agreed, and I don't think I have any substantial change requests for the > first two fixes. > > > >>> /* 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 > >> > > > -- > Chuck Lever >
On 3/20/25 4:46 PM, Olga Kornievskaia wrote: > On Thu, Mar 20, 2025 at 1:32 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 3/20/25 12:29 PM, Olga Kornievskaia wrote: >>> On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@oracle.com> wrote: >>>>> 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. >>> >>> Why would you think this check should go before? >> >> Because this code is confusing. >> >> So, for NLM, MAY_OWNER_OVERRIDE is already set for the uid_eq check. >> The order of the new code is not consequential. But it still catches >> the eye. > > I just re-checked the original code and the re(set)ing of acc was done > before the MAY_OWNER_OVERRIDE check. So changing my patch as you > suggest would be consistent with how things worked before. I will do > that. > >>> NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it >>> arrives in check_nfsd_access() . >> >> This code is /clearing/ the other flags too. That's a little subtle. >> >> The new comment should explain why only these two flags are needed for >> the subsequent code. > > inode_permission() only care about READ, WRITE (EXEC)? NFSD_MAY_READ > is supposed to be the same as MAY_READ etc. Thus I believe passing > other values is of no consequence so to be honest I don't understand > why NFSD_MAY_OWNER_OVERRIDE is needed here. But what is of consequence > is (not) passing a NFSD_MAY_WRITE (which is what gets passed in by > nlm_fopen()). > >> That is, explain not only why NFSD_MAY_READ is >> getting set, but also why NFSD_MAY_NLM and NFSD_MAY_BYPASS_GSS are >> getting cleared. >> >> Or, if clearing those flags was unintentional, make the code read: >> >> acc |= NFSD_MAY_READ; >> >> I don't understand this code well enough to figure out why nlm_fopen() >> shouldn't just set NFSD_MAY_READ. Therefore: better code comment needed. > > My comment came from the comment that was removed by commit > 4cc9b9f2bf4df. I don't have a good understanding of the code to > provide a "better" comment. > > nlm_fopen assigns the access based on the requested lock type. an > exclusive lock i'm assuming gets translated to the "write" = > NFSD_MAY_WRITE. But the the user might not have write access to the > file but still be allowed to request an exclusive lock on it, right? Agreed. > @@ -2531,16 +2531,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 The original comment isn't significantly more illuminating, true. This seems clearer to me, but folks who know this code better might feel it is a bit obvious: /* * For the purpose of permission checking NLM requests, * the locker must have READ access or own the file. */ >>> Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ >>> | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets >>> NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | >>> NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't >>> NLM asking for write permissions on the file for locking? >>> >>> My attempt was to return the code to previous functionality which >>> worked (and was only explicitly asking for read permissions on the >>> file). >> >> So the patch is reverting part of 4cc9b9f2bf4df; perhaps that should be >> called out in the patch description (up to you). However, the proposed >> fix doesn't make this code easier to understand, and that's probably my >> main quibble. >> >> >>> Unless you think more is needed here: I would resubmit with 3 patches >>> for each of the chunks and corresponding problems. >> >> Agreed, and I don't think I have any substantial change requests for the >> first two fixes. >> >> >>>>> /* 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 >>>> >> >> >> -- >> Chuck Lever >> >
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; + 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, 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; + /* 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));
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. 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. Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK") Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> --- fs/nfsd/export.c | 23 +++++++++++++---------- fs/nfsd/vfs.c | 4 ++++ 2 files changed, 17 insertions(+), 10 deletions(-)