On 2/20/2025 12:33 PM, Stephen Smalley wrote:

On Thu, Feb 20, 2025 at 3:31 PM Casey Schaufler [off-list ref] wrote:

quoted

On 2/20/2025 11:37 AM, Stephen Smalley wrote:

quoted

On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler [off-list ref] wrote:

quoted

On 2/20/2025 10:16 AM, Stephen Smalley wrote:

quoted

On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
[off-list ref] wrote:

quoted

On Thu, Feb 20, 2025 at 12:54 PM Paul Moore [off-list ref] wrote:

quoted

On Thu, Feb 20, 2025 at 12:40 PM Paul Moore [off-list ref] wrote:

quoted

On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
[off-list ref] wrote:

quoted

On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler [off-list ref] wrote:

quoted

Replace the (secctx,seclen) pointer pair with a single lsm_context
pointer to allow return of the LSM identifier along with the context
and context length. This allows security_release_secctx() to know how
to release the context. Callers have been modified to use or save the
returned data from the new structure.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: ceph-devel@vger.kernel.org
Cc: linux-nfs@vger.kernel.org
---
 fs/ceph/super.h               |  3 +--
 fs/ceph/xattr.c               | 16 ++++++----------
 fs/fuse/dir.c                 | 35 ++++++++++++++++++-----------------
 fs/nfs/nfs4proc.c             | 20 ++++++++++++--------
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      | 26 +++-----------------------
 security/security.c           |  9 ++++-----
 security/selinux/hooks.c      |  9 +++++----
 8 files changed, 50 insertions(+), 70 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 76776d716744..0b116ef3a752 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c

@@ -114,6 +114,7 @@ static inline struct nfs4_label *
 nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
        struct iattr *sattr, struct nfs4_label *label)
 {
+       struct lsm_context shim;
        int err;

        if (label == NULL)

@@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
        label->label = NULL;

        err = security_dentry_init_security(dentry, sattr->ia_mode,
-                               &dentry->d_name, NULL,
-                               (void **)&label->label, &label->len);
-       if (err == 0)
-               return label;
+                               &dentry->d_name, NULL, &shim);
+       if (err)
+               return NULL;

-       return NULL;
+       label->label = shim.context;
+       label->len = shim.len;
+       return label;
 }
 static inline void
 nfs4_label_release_security(struct nfs4_label *label)
 {
-       struct lsm_context scaff; /* scaffolding */
+       struct lsm_context shim;

        if (label) {
-               lsmcontext_init(&scaff, label->label, label->len, 0);
-               security_release_secctx(&scaff);
+               shim.context = label->label;
+               shim.len = label->len;
+               shim.id = LSM_ID_UNDEF;

Is there a patch that follows this one to fix this? Otherwise, setting
this to UNDEF causes SELinux to NOT free the context, which produces a
memory leak for every NFS inode security context. Reported by kmemleak
when running the selinux-testsuite NFS tests.

I don't recall seeing anything related to this, but patches are
definitely welcome.

Looking at this quickly, this is an interesting problem as I don't
believe we have enough context in nfs4_label_release_security() to
correctly set the shim.id value.  If there is a positive, it is that
lsm_context is really still just a string wrapped up with some
metadata, e.g. length/ID, so we kfree()'ing shim.context is going to
be okay-ish, at least for the foreseeable future.

I can think of two ways to fix this, but I'd love to hear other ideas too.

1. Handle the LSM_ID_UNDEF case directly in security_release_secctx()
and skip any individual LSM processing.

2. Define a new LSM_ID_ANY value and update all of the LSMs to also
process the ANY case as well as their own.

I'm not finding either option very exciting, but option #2 looks
particularly ugly, so I think I'd prefer to see someone draft a patch
for option #1 assuming nothing better is presented.

We could perhaps add a u32 lsmid to struct nfs4_label, save it from
the shim.id obtained in nfs4_label_init_security(), and use it in
nfs4_label_release_security(). Not sure why that wasn't done in the
first place.

Something like this (not tested yet). If this looks sane, will submit
separately.

commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security")
did not preserve the lsm id for subsequent release calls, which results
in a memory leak. Fix it by saving the lsm id in the nfs4_label and
providing it on the subsequent release call.

Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security")
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

I'm not a fan of adding secids into other subsystems, especially in cases
where they've tried to avoid them in the past.

The better solution, which I'm tracking down the patch for now, is for
the individual LSMs to always do their release, and for security_release_secctx()
to check the lsm_id and call the appropriate LSM specific hook. Until there
are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match.

Please don't use this patch.

It doesn't add a secid; it just saves the LSM id obtained from
lsm_context populated by the security_dentry_init_security() hook call
and passes it back in the lsm_context to the security_release_secctx()
call.

Right. Sorry. If you're going to do that, the nfs_label struct should
just include a lsm_context instead. But that hit opposition when proposed
initially.

The practical solution has to acknowledge that at this stage there can only
be one LSM providing contexts, and each LSM can release the context if the
LSM is matches the LSM or is LSM_ID_UNDEF. That will change before SELinux,
AppArmor and Smack can co-exist, but that's not yet available. For now the
check

        if (cp->id == LSM_ID_SELINUX)

can either be removed or changed to

        if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF)

In a system that respects LSM_FLAG_LEGACY_MAJOR the id isn't relevant
with the context using LSMs all being thus identified.

Shrug. My patch seemed cleaner,

Adding the lsm_context was cleaner. Not worth yet another roadblock.
I will have a patch asap. I'm dealing with a facilities issue at
Smack Labs (whole site being painted, everything in disarray) that
is slowing things down.

 but I don't really care as long as it
is fixed, preferably before 6.14 goes final.