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

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.

quoted

---
 fs/nfs/nfs4proc.c    | 7 ++++---
 include/linux/nfs4.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index df9669d4ded7..c0caaec7bd20 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c

@@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct

dentry *dentry,
  if (err)
  return NULL;

+ label->lsmid = shim.id;
  label->label = shim.context;
  label->len = shim.len;
  return label;

@@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label)
  if (label) {
  shim.context = label->label;
  shim.len = label->len;
- shim.id = LSM_ID_UNDEF;
+ shim.id = label->lsmid;
  security_release_secctx(&shim);
  }
 }

@@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode

*inode, void *buf,
  size_t buflen)
 {
  struct nfs_server *server = NFS_SERVER(inode);
- struct nfs4_label label = {0, 0, buflen, buf};
+ struct nfs4_label label = {0, 0, 0, buflen, buf};

  u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL };
  struct nfs_fattr fattr = {

@@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode,
 static int
 nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen)
 {
- struct nfs4_label ilabel = {0, 0, buflen, (char *)buf };
+ struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf };
  struct nfs_fattr *fattr;
  int status;

diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 71fbebfa43c7..9ac83ca88326 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h

@@ -47,6 +47,7 @@ struct nfs4_acl {
 struct nfs4_label {
  uint32_t lfs;
  uint32_t pi;
+ u32 lsmid;
  u32 len;
  char *label;
 };