Re: [PATCH v3 4/5] LSM: lsm_context in security_dentry_init_security

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

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(-)

quoted

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>
---
 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;
 };

-- 
2.48.1