[PATCH v3 0/5] LSM: Replace secctx/len pairs with lsm_context

[PATCH v3 0/5] LSM: Replace secctx/len pairs with lsm_context

[Casey Schaufler]

LSM: Replace secctx/len pairs with lsm_context

Several of the Linux Security Module (LSM) interfaces use a pair of
pointers for transmitting security context data and data length. The
data passed is refered to as a security context.  While all existing
modules provide nul terminated strings, there is no requirement that
they to so. Hence, the length is necessary.

Security contexts are provided by a number of interfaces. The interface
security_release_secctx() is used when the caller is finished with the
data. Each of the security modules that provide security contexts manages
them differently. This was safe in the past, because only one security
module that provides security contexts is allowed to be active. To allow
multiple active modules that use security contexts it is necessary to
identify which security module created a security context. Adding a third
pointer to the interfaces for the LSM identification is not appealing.

A new structure, lsm_context, is created for use in these interfaces.
It includes three members: the data pointer, the data length and
the LSM ID of its creator. The interfaces that create contexts and
security_release_secctx() now use a pointer to an lsm_context instead
of a pointer pair.

The changes are mostly mechanical, and some scaffolding is used within
the patch set to allow for smaller individual patches.

This patch set depends on the patch set LSM: Move away from secids:
        https://github.com/cschaufler/lsm-stacking.git#lsmprop-6.12-rc1-v4

https://github.com/cschaufler/lsm-stacking.git#context-6.12-rc1-v3

Revisons:
	v3: Don't change NFS data storage format
	    Retain argument alignments
	    Set released context pointers to NULL
        v2: Rebase for static calls in LSM infrastructure

Casey Schaufler (5):
  LSM: Ensure the correct LSM context releaser
  LSM: Replace context+len with lsm_context
  LSM: Use lsm_context in security_inode_getsecctx
  LSM: lsm_context in security_dentry_init_security
  LSM: secctx provider check on release

 drivers/android/binder.c                | 25 +++++-----
 fs/ceph/super.h                         |  3 +-
 fs/ceph/xattr.c                         | 12 ++---
 fs/fuse/dir.c                           | 35 +++++++-------
 fs/nfs/nfs4proc.c                       | 22 ++++++---
 fs/nfsd/nfs4xdr.c                       | 22 ++++-----
 include/linux/lsm_hook_defs.h           | 13 +++--
 include/linux/security.h                | 37 ++++++++++-----
 include/net/scm.h                       | 12 ++---
 kernel/audit.c                          | 33 +++++++------
 kernel/auditsc.c                        | 27 +++++------
 net/ipv4/ip_sockglue.c                  | 12 ++---
 net/netfilter/nf_conntrack_netlink.c    | 16 +++----
 net/netfilter/nf_conntrack_standalone.c | 11 ++---
 net/netfilter/nfnetlink_queue.c         | 22 ++++-----
 net/netlabel/netlabel_unlabeled.c       | 44 +++++++----------
 net/netlabel/netlabel_user.c            | 10 ++--
 security/apparmor/include/secid.h       |  7 ++-
 security/apparmor/secid.c               | 34 +++++++------
 security/security.c                     | 63 ++++++++++++-------------
 security/selinux/hooks.c                | 50 ++++++++++++++------
 security/smack/smack_lsm.c              | 49 +++++++++++--------
 22 files changed, 293 insertions(+), 266 deletions(-)

-- 
2.46.0

[PATCH v3 1/5] LSM: Ensure the correct LSM context releaser

[Casey Schaufler]

Add a new lsm_context data structure to hold all the information about a
"security context", including the string, its size and which LSM allocated
the string. The allocation information is necessary because LSMs have
different policies regarding the lifecycle of these strings. SELinux
allocates and destroys them on each use, whereas Smack provides a pointer
to an entry in a list that never goes away.

Update security_release_secctx() to use the lsm_context instead of a
(char *, len) pair. Change its callers to do likewise.  The LSMs
supporting this hook have had comments added to remind the developer
that there is more work to be done.

The BPF security module provides all LSM hooks. While there has yet to
be a known instance of a BPF configuration that uses security contexts,
the possibility is real. In the existing implementation there is
potential for multiple frees in that case.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-integrity@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: audit@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: linux-nfs@vger.kernel.org
Cc: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c                | 24 +++++++--------
 fs/ceph/xattr.c                         |  6 +++-
 fs/nfs/nfs4proc.c                       |  8 +++--
 fs/nfsd/nfs4xdr.c                       |  8 +++--
 include/linux/lsm_hook_defs.h           |  2 +-
 include/linux/security.h                | 35 ++++++++++++++++++++--
 include/net/scm.h                       | 11 +++----
 kernel/audit.c                          | 30 +++++++++----------
 kernel/auditsc.c                        | 23 +++++++-------
 net/ipv4/ip_sockglue.c                  | 10 +++----
 net/netfilter/nf_conntrack_netlink.c    | 10 +++----
 net/netfilter/nf_conntrack_standalone.c |  9 +++---
 net/netfilter/nfnetlink_queue.c         | 13 +++++---
 net/netlabel/netlabel_unlabeled.c       | 40 +++++++++++--------------
 net/netlabel/netlabel_user.c            | 11 ++++---
 security/apparmor/include/secid.h       |  2 +-
 security/apparmor/secid.c               | 11 +++++--
 security/security.c                     |  8 ++---
 security/selinux/hooks.c                | 11 +++++--
 19 files changed, 165 insertions(+), 107 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 978740537a1a..d4229bf6f789 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3011,8 +3011,7 @@ static void binder_transaction(struct binder_proc *proc,
 	struct binder_context *context = proc->context;
 	int t_debug_id = atomic_inc_return(&binder_last_id);
 	ktime_t t_start_time = ktime_get();
-	char *secctx = NULL;
-	u32 secctx_sz = 0;
+	struct lsm_context lsmctx;
 	struct list_head sgc_head;
 	struct list_head pf_head;
 	const void __user *user_buffer = (const void __user *)
@@ -3291,7 +3290,8 @@ static void binder_transaction(struct binder_proc *proc,
 		size_t added_size;
 
 		security_cred_getsecid(proc->cred, &secid);
-		ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
+		ret = security_secid_to_secctx(secid, &lsmctx.context,
+					       &lsmctx.len);
 		if (ret) {
 			binder_txn_error("%d:%d failed to get security context\n",
 				thread->pid, proc->pid);
@@ -3300,7 +3300,7 @@ static void binder_transaction(struct binder_proc *proc,
 			return_error_line = __LINE__;
 			goto err_get_secctx_failed;
 		}
-		added_size = ALIGN(secctx_sz, sizeof(u64));
+		added_size = ALIGN(lsmctx.len, sizeof(u64));
 		extra_buffers_size += added_size;
 		if (extra_buffers_size < added_size) {
 			binder_txn_error("%d:%d integer overflow of extra_buffers_size\n",
@@ -3334,23 +3334,23 @@ static void binder_transaction(struct binder_proc *proc,
 		t->buffer = NULL;
 		goto err_binder_alloc_buf_failed;
 	}
-	if (secctx) {
+	if (lsmctx.context) {
 		int err;
 		size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
 				    ALIGN(tr->offsets_size, sizeof(void *)) +
 				    ALIGN(extra_buffers_size, sizeof(void *)) -
-				    ALIGN(secctx_sz, sizeof(u64));
+				    ALIGN(lsmctx.len, sizeof(u64));
 
 		t->security_ctx = t->buffer->user_data + buf_offset;
 		err = binder_alloc_copy_to_buffer(&target_proc->alloc,
 						  t->buffer, buf_offset,
-						  secctx, secctx_sz);
+						  lsmctx.context, lsmctx.len);
 		if (err) {
 			t->security_ctx = 0;
 			WARN_ON(1);
 		}
-		security_release_secctx(secctx, secctx_sz);
-		secctx = NULL;
+		security_release_secctx(&lsmctx);
+		lsmctx.context = NULL;
 	}
 	t->buffer->debug_id = t->debug_id;
 	t->buffer->transaction = t;
@@ -3394,7 +3394,7 @@ static void binder_transaction(struct binder_proc *proc,
 	off_end_offset = off_start_offset + tr->offsets_size;
 	sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
 	sg_buf_end_offset = sg_buf_offset + extra_buffers_size -
-		ALIGN(secctx_sz, sizeof(u64));
+		ALIGN(lsmctx.len, sizeof(u64));
 	off_min = 0;
 	for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
 	     buffer_offset += sizeof(binder_size_t)) {
@@ -3773,8 +3773,8 @@ static void binder_transaction(struct binder_proc *proc,
 	binder_alloc_free_buf(&target_proc->alloc, t->buffer);
 err_binder_alloc_buf_failed:
 err_bad_extra_size:
-	if (secctx)
-		security_release_secctx(secctx, secctx_sz);
+	if (lsmctx.context)
+		security_release_secctx(&lsmctx);
 err_get_secctx_failed:
 	kfree(tcomplete);
 	binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index e066a556eccb..f7996770cc2c 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -1446,12 +1446,16 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 
 void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx)
 {
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+	struct lsm_context scaff; /* scaffolding */
+#endif
 #ifdef CONFIG_CEPH_FS_POSIX_ACL
 	posix_acl_release(as_ctx->acl);
 	posix_acl_release(as_ctx->default_acl);
 #endif
 #ifdef CONFIG_CEPH_FS_SECURITY_LABEL
-	security_release_secctx(as_ctx->sec_ctx, as_ctx->sec_ctxlen);
+	lsmcontext_init(&scaff, as_ctx->sec_ctx, as_ctx->sec_ctxlen, 0);
+	security_release_secctx(&scaff);
 #endif
 #ifdef CONFIG_FS_ENCRYPTION
 	kfree(as_ctx->fscrypt_auth);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cd2fbde2e6d7..76776d716744 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -138,8 +138,12 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
 static inline void
 nfs4_label_release_security(struct nfs4_label *label)
 {
-	if (label)
-		security_release_secctx(label->label, label->len);
+	struct lsm_context scaff; /* scaffolding */
+
+	if (label) {
+		lsmcontext_init(&scaff, label->label, label->len, 0);
+		security_release_secctx(&scaff);
+	}
 }
 static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
 {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f118921250c3..537ad363d70a 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3653,8 +3653,12 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 
 out:
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	if (args.context)
-		security_release_secctx(args.context, args.contextlen);
+	if (args.context) {
+		struct lsm_context scaff; /* scaffolding */
+
+		lsmcontext_init(&scaff, args.context, args.contextlen, 0);
+		security_release_secctx(&scaff);
+	}
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 	kfree(args.acl);
 	if (tempfh) {
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eb2937599cb0..c13df23132eb 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -300,7 +300,7 @@ LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
 LSM_HOOK(int, -EOPNOTSUPP, lsmprop_to_secctx, struct lsm_prop *prop,
 	 char **secdata, u32 *seclen)
 LSM_HOOK(int, 0, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
-LSM_HOOK(void, LSM_RET_VOID, release_secctx, char *secdata, u32 seclen)
+LSM_HOOK(void, LSM_RET_VOID, release_secctx, struct lsm_context *cp)
 LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
 LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
 LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
diff --git a/include/linux/security.h b/include/linux/security.h
index fd690fa73162..1a3ca02224e8 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -225,6 +225,37 @@ extern unsigned long dac_mmap_min_addr;
 #define dac_mmap_min_addr	0UL
 #endif
 
+/*
+ * A "security context" is the text representation of
+ * the information used by LSMs.
+ * This structure contains the string, its length, and which LSM
+ * it is useful for.
+ */
+struct lsm_context {
+	char	*context;	/* Provided by the module */
+	u32	len;
+	int	id;		/* Identifies the module */
+};
+
+/**
+ * lsmcontext_init - initialize an lsmcontext structure.
+ * @cp: Pointer to the context to initialize
+ * @context: Initial context, or NULL
+ * @size: Size of context, or 0
+ * @id: Which LSM provided the context
+ *
+ * Fill in the lsmcontext from the provided information.
+ * This is a scaffolding function that will be removed when
+ * lsm_context integration is complete.
+ */
+static inline void lsmcontext_init(struct lsm_context *cp, char *context,
+				   u32 size, int id)
+{
+	cp->id = id;
+	cp->context = context;
+	cp->len = size;
+}
+
 /*
  * Values used in the task_security_ops calls
  */
@@ -556,7 +587,7 @@ int security_ismaclabel(const char *name);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
 int security_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata, u32 *seclen);
 int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
-void security_release_secctx(char *secdata, u32 seclen);
+void security_release_secctx(struct lsm_context *cp);
 void security_inode_invalidate_secctx(struct inode *inode);
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -1545,7 +1576,7 @@ static inline int security_secctx_to_secid(const char *secdata,
 	return -EOPNOTSUPP;
 }
 
-static inline void security_release_secctx(char *secdata, u32 seclen)
+static inline void security_release_secctx(struct lsm_context *cp)
 {
 }
 
diff --git a/include/net/scm.h b/include/net/scm.h
index 0d35c7c77a74..f75449e1d876 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -105,16 +105,17 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
 #ifdef CONFIG_SECURITY_NETWORK
 static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
 {
-	char *secdata;
-	u32 seclen;
+	struct lsm_context ctx;
 	int err;
 
 	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
-		err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
+		err = security_secid_to_secctx(scm->secid, &ctx.context,
+					       &ctx.len);
 
 		if (!err) {
-			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
-			security_release_secctx(secdata, seclen);
+			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, ctx.len,
+				 ctx.context);
+			security_release_secctx(&ctx);
 		}
 	}
 }
diff --git a/kernel/audit.c b/kernel/audit.c
index d2797e8fe182..bafd8fd2b1f3 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1221,8 +1221,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct audit_buffer	*ab;
 	u16			msg_type = nlh->nlmsg_type;
 	struct audit_sig_info   *sig_data;
-	char			*ctx = NULL;
-	u32			len;
+	struct lsm_context	lsmctx;
 
 	err = audit_netlink_ok(skb, msg_type);
 	if (err)
@@ -1472,27 +1471,29 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		break;
 	}
 	case AUDIT_SIGNAL_INFO:
-		len = 0;
 		if (lsmprop_is_set(&audit_sig_lsm)) {
-			err = security_lsmprop_to_secctx(&audit_sig_lsm, &ctx,
-							 &len);
+			err = security_lsmprop_to_secctx(&audit_sig_lsm,
+							 &lsmctx.context,
+							 &lsmctx.len);
 			if (err)
 				return err;
 		}
-		sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL);
+		sig_data = kmalloc(struct_size(sig_data, ctx, lsmctx.len),
+				   GFP_KERNEL);
 		if (!sig_data) {
 			if (lsmprop_is_set(&audit_sig_lsm))
-				security_release_secctx(ctx, len);
+				security_release_secctx(&lsmctx);
 			return -ENOMEM;
 		}
 		sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
 		sig_data->pid = audit_sig_pid;
 		if (lsmprop_is_set(&audit_sig_lsm)) {
-			memcpy(sig_data->ctx, ctx, len);
-			security_release_secctx(ctx, len);
+			memcpy(sig_data->ctx, lsmctx.context, lsmctx.len);
+			security_release_secctx(&lsmctx);
 		}
 		audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
-				 sig_data, struct_size(sig_data, ctx, len));
+				 sig_data, struct_size(sig_data, ctx,
+						       lsmctx.len));
 		kfree(sig_data);
 		break;
 	case AUDIT_TTY_GET: {
@@ -2180,23 +2181,22 @@ void audit_log_key(struct audit_buffer *ab, char *key)
 int audit_log_task_context(struct audit_buffer *ab)
 {
 	struct lsm_prop prop;
-	char *ctx = NULL;
-	unsigned len;
+	struct lsm_context ctx;
 	int error;
 
 	security_current_getlsmprop_subj(&prop);
 	if (!lsmprop_is_set(&prop))
 		return 0;
 
-	error = security_lsmprop_to_secctx(&prop, &ctx, &len);
+	error = security_lsmprop_to_secctx(&prop, &ctx.context, &ctx.len);
 	if (error) {
 		if (error != -EINVAL)
 			goto error_path;
 		return 0;
 	}
 
-	audit_log_format(ab, " subj=%s", ctx);
-	security_release_secctx(ctx, len);
+	audit_log_format(ab, " subj=%s", ctx.context);
+	security_release_secctx(&ctx);
 	return 0;
 
 error_path:
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f28fd513d047..c196dd4c9b54 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1098,8 +1098,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 				 char *comm)
 {
 	struct audit_buffer *ab;
-	char *ctx = NULL;
-	u32 len;
+	struct lsm_context ctx;
 	int rc = 0;
 
 	ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
@@ -1110,12 +1109,12 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 			 from_kuid(&init_user_ns, auid),
 			 from_kuid(&init_user_ns, uid), sessionid);
 	if (lsmprop_is_set(prop)) {
-		if (security_lsmprop_to_secctx(prop, &ctx, &len)) {
+		if (security_lsmprop_to_secctx(prop, &ctx.context, &ctx.len)) {
 			audit_log_format(ab, " obj=(none)");
 			rc = 1;
 		} else {
-			audit_log_format(ab, " obj=%s", ctx);
-			security_release_secctx(ctx, len);
+			audit_log_format(ab, " obj=%s", ctx.context);
+			security_release_secctx(&ctx);
 		}
 	}
 	audit_log_format(ab, " ocomm=");
@@ -1371,6 +1370,7 @@ static void audit_log_time(struct audit_context *context, struct audit_buffer **
 
 static void show_special(struct audit_context *context, int *call_panic)
 {
+	struct lsm_context lsmcxt;
 	struct audit_buffer *ab;
 	int i;
 
@@ -1401,7 +1401,8 @@ static void show_special(struct audit_context *context, int *call_panic)
 				*call_panic = 1;
 			} else {
 				audit_log_format(ab, " obj=%s", ctx);
-				security_release_secctx(ctx, len);
+				lsmcontext_init(&lsmcxt, ctx, len, 0);
+				security_release_secctx(&lsmcxt);
 			}
 		}
 		if (context->ipc.has_perm) {
@@ -1560,15 +1561,15 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
 				 MAJOR(n->rdev),
 				 MINOR(n->rdev));
 	if (lsmprop_is_set(&n->oprop)) {
-		char *ctx = NULL;
-		u32 len;
+		struct lsm_context ctx;
 
-		if (security_lsmprop_to_secctx(&n->oprop, &ctx, &len)) {
+		if (security_lsmprop_to_secctx(&n->oprop, &ctx.context,
+					       &ctx.len)) {
 			if (call_panic)
 				*call_panic = 2;
 		} else {
-			audit_log_format(ab, " obj=%s", ctx);
-			security_release_secctx(ctx, len);
+			audit_log_format(ab, " obj=%s", ctx.context);
+			security_release_secctx(&ctx);
 		}
 	}
 
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index cf377377b52d..a8180dcc2a32 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -128,20 +128,20 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
 
 static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
 {
-	char *secdata;
-	u32 seclen, secid;
+	struct lsm_context ctx;
+	u32 secid;
 	int err;
 
 	err = security_socket_getpeersec_dgram(NULL, skb, &secid);
 	if (err)
 		return;
 
-	err = security_secid_to_secctx(secid, &secdata, &seclen);
+	err = security_secid_to_secctx(secid, &ctx.context, &ctx.len);
 	if (err)
 		return;
 
-	put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
-	security_release_secctx(secdata, seclen);
+	put_cmsg(msg, SOL_IP, SCM_SECURITY, ctx.len, ctx.context);
+	security_release_secctx(&ctx);
 }
 
 static void ip_cmsg_recv_dstaddr(struct msghdr *msg, struct sk_buff *skb)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 6a1239433830..86a57a3afdd6 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -357,10 +357,10 @@ static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct,
 static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
 {
 	struct nlattr *nest_secctx;
-	int len, ret;
-	char *secctx;
+	struct lsm_context ctx;
+	int ret;
 
-	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
 	if (ret)
 		return 0;
 
@@ -369,13 +369,13 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
 	if (!nest_secctx)
 		goto nla_put_failure;
 
-	if (nla_put_string(skb, CTA_SECCTX_NAME, secctx))
+	if (nla_put_string(skb, CTA_SECCTX_NAME, ctx.context))
 		goto nla_put_failure;
 	nla_nest_end(skb, nest_secctx);
 
 	ret = 0;
 nla_put_failure:
-	security_release_secctx(secctx, len);
+	security_release_secctx(&ctx);
 	return ret;
 }
 #else
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 7d4f0fa8b609..5f7fd23b7afe 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -172,17 +172,16 @@ static void ct_seq_stop(struct seq_file *s, void *v)
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
 static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
 {
+	struct lsm_context ctx;
 	int ret;
-	u32 len;
-	char *secctx;
 
-	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
 	if (ret)
 		return;
 
-	seq_printf(s, "secctx=%s ", secctx);
+	seq_printf(s, "secctx=%s ", ctx.context);
 
-	security_release_secctx(secctx, len);
+	security_release_secctx(&ctx);
 }
 #else
 static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index d2773ce9b585..37757cd77cf1 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -567,6 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	enum ip_conntrack_info ctinfo = 0;
 	const struct nfnl_ct_hook *nfnl_ct;
 	bool csum_verify;
+	struct lsm_context scaff; /* scaffolding */
 	char *secdata = NULL;
 	u32 seclen = 0;
 	ktime_t tstamp;
@@ -810,8 +811,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	}
 
 	nlh->nlmsg_len = skb->len;
-	if (seclen)
-		security_release_secctx(secdata, seclen);
+	if (seclen) {
+		lsmcontext_init(&scaff, secdata, seclen, 0);
+		security_release_secctx(&scaff);
+	}
 	return skb;
 
 nla_put_failure:
@@ -819,8 +822,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	kfree_skb(skb);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 nlmsg_failure:
-	if (seclen)
-		security_release_secctx(secdata, seclen);
+	if (seclen) {
+		lsmcontext_init(&scaff, secdata, seclen, 0);
+		security_release_secctx(&scaff);
+	}
 	return NULL;
 }
 
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 1bc2d0890a9f..921fa8eeb451 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -374,8 +374,7 @@ int netlbl_unlhsh_add(struct net *net,
 	struct net_device *dev;
 	struct netlbl_unlhsh_iface *iface;
 	struct audit_buffer *audit_buf = NULL;
-	char *secctx = NULL;
-	u32 secctx_len;
+	struct lsm_context ctx;
 
 	if (addr_len != sizeof(struct in_addr) &&
 	    addr_len != sizeof(struct in6_addr))
@@ -439,10 +438,10 @@ int netlbl_unlhsh_add(struct net *net,
 	rcu_read_unlock();
 	if (audit_buf != NULL) {
 		if (security_secid_to_secctx(secid,
-					     &secctx,
-					     &secctx_len) == 0) {
-			audit_log_format(audit_buf, " sec_obj=%s", secctx);
-			security_release_secctx(secctx, secctx_len);
+					     &ctx.context,
+					     &ctx.len) == 0) {
+			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
+			security_release_secctx(&ctx);
 		}
 		audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
 		audit_log_end(audit_buf);
@@ -473,8 +472,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
 	struct netlbl_unlhsh_addr4 *entry;
 	struct audit_buffer *audit_buf;
 	struct net_device *dev;
-	char *secctx;
-	u32 secctx_len;
+	struct lsm_context ctx;
 
 	spin_lock(&netlbl_unlhsh_lock);
 	list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
@@ -495,9 +493,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
 		dev_put(dev);
 		if (entry != NULL &&
 		    security_secid_to_secctx(entry->secid,
-					     &secctx, &secctx_len) == 0) {
-			audit_log_format(audit_buf, " sec_obj=%s", secctx);
-			security_release_secctx(secctx, secctx_len);
+					     &ctx.context, &ctx.len) == 0) {
+			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
+			security_release_secctx(&ctx);
 		}
 		audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
 		audit_log_end(audit_buf);
@@ -534,8 +532,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
 	struct netlbl_unlhsh_addr6 *entry;
 	struct audit_buffer *audit_buf;
 	struct net_device *dev;
-	char *secctx;
-	u32 secctx_len;
+	struct lsm_context ctx;
 
 	spin_lock(&netlbl_unlhsh_lock);
 	list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
@@ -555,9 +552,9 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
 		dev_put(dev);
 		if (entry != NULL &&
 		    security_secid_to_secctx(entry->secid,
-					     &secctx, &secctx_len) == 0) {
-			audit_log_format(audit_buf, " sec_obj=%s", secctx);
-			security_release_secctx(secctx, secctx_len);
+					     &ctx.context, &ctx.len) == 0) {
+			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
+			security_release_secctx(&ctx);
 		}
 		audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
 		audit_log_end(audit_buf);
@@ -1069,10 +1066,9 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
 	int ret_val = -ENOMEM;
 	struct netlbl_unlhsh_walk_arg *cb_arg = arg;
 	struct net_device *dev;
+	struct lsm_context ctx;
 	void *data;
 	u32 secid;
-	char *secctx;
-	u32 secctx_len;
 
 	data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
 			   cb_arg->seq, &netlbl_unlabel_gnl_family,
@@ -1127,14 +1123,14 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
 		secid = addr6->secid;
 	}
 
-	ret_val = security_secid_to_secctx(secid, &secctx, &secctx_len);
+	ret_val = security_secid_to_secctx(secid, &ctx.context, &ctx.len);
 	if (ret_val != 0)
 		goto list_cb_failure;
 	ret_val = nla_put(cb_arg->skb,
 			  NLBL_UNLABEL_A_SECCTX,
-			  secctx_len,
-			  secctx);
-	security_release_secctx(secctx, secctx_len);
+			  ctx.len,
+			  ctx.context);
+	security_release_secctx(&ctx);
 	if (ret_val != 0)
 		goto list_cb_failure;
 
diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
index 81635a13987b..f5e7a9919df1 100644
--- a/net/netlabel/netlabel_user.c
+++ b/net/netlabel/netlabel_user.c
@@ -84,8 +84,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
 					       struct netlbl_audit *audit_info)
 {
 	struct audit_buffer *audit_buf;
-	char *secctx;
-	u32 secctx_len;
+	struct lsm_context ctx;
 
 	if (audit_enabled == AUDIT_OFF)
 		return NULL;
@@ -99,10 +98,10 @@ struct audit_buffer *netlbl_audit_start_common(int type,
 			 audit_info->sessionid);
 
 	if (lsmprop_is_set(&audit_info->prop) &&
-	    security_lsmprop_to_secctx(&audit_info->prop, &secctx,
-				       &secctx_len) == 0) {
-		audit_log_format(audit_buf, " subj=%s", secctx);
-		security_release_secctx(secctx, secctx_len);
+	    security_lsmprop_to_secctx(&audit_info->prop, &ctx.context,
+				       &ctx.len) == 0) {
+		audit_log_format(audit_buf, " subj=%s", ctx.context);
+		security_release_secctx(&ctx);
 	}
 
 	return audit_buf;
diff --git a/security/apparmor/include/secid.h b/security/apparmor/include/secid.h
index cc6d1c9f4a47..8b92f90b6921 100644
--- a/security/apparmor/include/secid.h
+++ b/security/apparmor/include/secid.h
@@ -29,7 +29,7 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
 int apparmor_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
 			       u32 *seclen);
 int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
-void apparmor_release_secctx(char *secdata, u32 seclen);
+void apparmor_release_secctx(struct lsm_context *cp);
 
 
 int aa_alloc_secid(struct aa_label *label, gfp_t gfp);
diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
index 6350d107013a..8d9ced8cdffd 100644
--- a/security/apparmor/secid.c
+++ b/security/apparmor/secid.c
@@ -120,9 +120,16 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 	return 0;
 }
 
-void apparmor_release_secctx(char *secdata, u32 seclen)
+void apparmor_release_secctx(struct lsm_context *cp)
 {
-	kfree(secdata);
+	/*
+	 * stacking scaffolding:
+	 * When it is possible for more than one LSM to provide a
+	 * release hook, do this check:
+	 * if (cp->id == LSM_ID_APPARMOR || cp->id == LSM_ID_UNDEF)
+	 */
+
+	kfree(cp->context);
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index 0003d5ace5cc..0c9c3a02704b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4365,14 +4365,14 @@ EXPORT_SYMBOL(security_secctx_to_secid);
 
 /**
  * security_release_secctx() - Free a secctx buffer
- * @secdata: secctx
- * @seclen: length of secctx
+ * @cp: the security context
  *
  * Release the security context.
  */
-void security_release_secctx(char *secdata, u32 seclen)
+void security_release_secctx(struct lsm_context *cp)
 {
-	call_void_hook(release_secctx, secdata, seclen);
+	call_void_hook(release_secctx, cp);
+	memset(cp, 0, sizeof(*cp));
 }
 EXPORT_SYMBOL(security_release_secctx);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 025b60c5b605..1503d398c87d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6624,9 +6624,16 @@ static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 				       secid, GFP_KERNEL);
 }
 
-static void selinux_release_secctx(char *secdata, u32 seclen)
+static void selinux_release_secctx(struct lsm_context *cp)
 {
-	kfree(secdata);
+	/*
+	 * stacking scaffolding:
+	 * When it is possible for more than one LSM to provide a
+	 * release hook, do this check:
+	 * if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF)
+	 */
+
+	kfree(cp->context);
 }
 
 static void selinux_inode_invalidate_secctx(struct inode *inode)
-- 
2.46.0

Re: [PATCH v3 1/5] LSM: Ensure the correct LSM context releaser

[Paul Moore]

On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> Add a new lsm_context data structure to hold all the information about a
> "security context", including the string, its size and which LSM allocated
> the string. The allocation information is necessary because LSMs have
> different policies regarding the lifecycle of these strings. SELinux
> allocates and destroys them on each use, whereas Smack provides a pointer
> to an entry in a list that never goes away.
> 
> Update security_release_secctx() to use the lsm_context instead of a
> (char *, len) pair. Change its callers to do likewise.  The LSMs
> supporting this hook have had comments added to remind the developer
> that there is more work to be done.
> 
> The BPF security module provides all LSM hooks. While there has yet to
> be a known instance of a BPF configuration that uses security contexts,
> the possibility is real. In the existing implementation there is
> potential for multiple frees in that case.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: audit@vger.kernel.org
> Cc: netfilter-devel@vger.kernel.org
> To: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: linux-nfs@vger.kernel.org
> Cc: Todd Kjos <tkjos@google.com>
> ---
>  drivers/android/binder.c                | 24 +++++++--------
>  fs/ceph/xattr.c                         |  6 +++-
>  fs/nfs/nfs4proc.c                       |  8 +++--
>  fs/nfsd/nfs4xdr.c                       |  8 +++--
>  include/linux/lsm_hook_defs.h           |  2 +-
>  include/linux/security.h                | 35 ++++++++++++++++++++--
>  include/net/scm.h                       | 11 +++----
>  kernel/audit.c                          | 30 +++++++++----------
>  kernel/auditsc.c                        | 23 +++++++-------
>  net/ipv4/ip_sockglue.c                  | 10 +++----
>  net/netfilter/nf_conntrack_netlink.c    | 10 +++----
>  net/netfilter/nf_conntrack_standalone.c |  9 +++---
>  net/netfilter/nfnetlink_queue.c         | 13 +++++---
>  net/netlabel/netlabel_unlabeled.c       | 40 +++++++++++--------------
>  net/netlabel/netlabel_user.c            | 11 ++++---
>  security/apparmor/include/secid.h       |  2 +-
>  security/apparmor/secid.c               | 11 +++++--
>  security/security.c                     |  8 ++---
>  security/selinux/hooks.c                | 11 +++++--
>  19 files changed, 165 insertions(+), 107 deletions(-)

This revision looks okay to me, and with no real comments from the other
affected subsystems on this or the previous revision I'm going to go
ahead and merge this into the lsm/dev branch.

Thanks Casey.

--
paul-moore.com

Re: [PATCH v3 1/5] LSM: Ensure the correct LSM context releaser

[Kees Bakker]

Op 23-10-2024 om 23:21 schreef Casey Schaufler:
> Add a new lsm_context data structure to hold all the information about a
> "security context", including the string, its size and which LSM allocated
> the string. The allocation information is necessary because LSMs have
> different policies regarding the lifecycle of these strings. SELinux
> allocates and destroys them on each use, whereas Smack provides a pointer
> to an entry in a list that never goes away.
>
> Update security_release_secctx() to use the lsm_context instead of a
> (char *, len) pair. Change its callers to do likewise.  The LSMs
> supporting this hook have had comments added to remind the developer
> that there is more work to be done.
>
> The BPF security module provides all LSM hooks. While there has yet to
> be a known instance of a BPF configuration that uses security contexts,
> the possibility is real. In the existing implementation there is
> potential for multiple frees in that case.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: audit@vger.kernel.org
> Cc: netfilter-devel@vger.kernel.org
> To: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: linux-nfs@vger.kernel.org
> Cc: Todd Kjos <tkjos@google.com>
> ---
>   drivers/android/binder.c                | 24 +++++++--------
>   fs/ceph/xattr.c                         |  6 +++-
>   fs/nfs/nfs4proc.c                       |  8 +++--
>   fs/nfsd/nfs4xdr.c                       |  8 +++--
>   include/linux/lsm_hook_defs.h           |  2 +-
>   include/linux/security.h                | 35 ++++++++++++++++++++--
>   include/net/scm.h                       | 11 +++----
>   kernel/audit.c                          | 30 +++++++++----------
>   kernel/auditsc.c                        | 23 +++++++-------
>   net/ipv4/ip_sockglue.c                  | 10 +++----
>   net/netfilter/nf_conntrack_netlink.c    | 10 +++----
>   net/netfilter/nf_conntrack_standalone.c |  9 +++---
>   net/netfilter/nfnetlink_queue.c         | 13 +++++---
>   net/netlabel/netlabel_unlabeled.c       | 40 +++++++++++--------------
>   net/netlabel/netlabel_user.c            | 11 ++++---
>   security/apparmor/include/secid.h       |  2 +-
>   security/apparmor/secid.c               | 11 +++++--
>   security/security.c                     |  8 ++---
>   security/selinux/hooks.c                | 11 +++++--
>   19 files changed, 165 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 978740537a1a..d4229bf6f789 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3011,8 +3011,7 @@ static void binder_transaction(struct binder_proc *proc,
>   	struct binder_context *context = proc->context;
>   	int t_debug_id = atomic_inc_return(&binder_last_id);
>   	ktime_t t_start_time = ktime_get();
> -	char *secctx = NULL;
> -	u32 secctx_sz = 0;
> +	struct lsm_context lsmctx;
Not initialized ?
>   	struct list_head sgc_head;
>   	struct list_head pf_head;
>   	const void __user *user_buffer = (const void __user *)
> @@ -3291,7 +3290,8 @@ static void binder_transaction(struct binder_proc *proc,
>   		size_t added_size;
>   
>   		security_cred_getsecid(proc->cred, &secid);
> -		ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> +		ret = security_secid_to_secctx(secid, &lsmctx.context,
> +					       &lsmctx.len);
>   		if (ret) {
>   			binder_txn_error("%d:%d failed to get security context\n",
>   				thread->pid, proc->pid);
> @@ -3300,7 +3300,7 @@ static void binder_transaction(struct binder_proc *proc,
>   			return_error_line = __LINE__;
>   			goto err_get_secctx_failed;
>   		}
> -		added_size = ALIGN(secctx_sz, sizeof(u64));
> +		added_size = ALIGN(lsmctx.len, sizeof(u64));
>   		extra_buffers_size += added_size;
>   		if (extra_buffers_size < added_size) {
>   			binder_txn_error("%d:%d integer overflow of extra_buffers_size\n",
> @@ -3334,23 +3334,23 @@ static void binder_transaction(struct binder_proc *proc,
>   		t->buffer = NULL;
>   		goto err_binder_alloc_buf_failed;
>   	}
> -	if (secctx) {
> +	if (lsmctx.context) {
 From code inspection it is not immediately obvious. Can you
guarantee that lsmctx is always initialized when the code
gets to this point? Perhaps it is safer to just initialize when
it is defined above (line 3014).

Re: [PATCH v3 1/5] LSM: Ensure the correct LSM context releaser

[Casey Schaufler]

On 12/6/2024 12:05 PM, Kees Bakker wrote:
> Op 23-10-2024 om 23:21 schreef Casey Schaufler:
>> Add a new lsm_context data structure to hold all the information about a
>> "security context", including the string, its size and which LSM
>> allocated
>> the string. The allocation information is necessary because LSMs have
>> different policies regarding the lifecycle of these strings. SELinux
>> allocates and destroys them on each use, whereas Smack provides a
>> pointer
>> to an entry in a list that never goes away.
>>
>> Update security_release_secctx() to use the lsm_context instead of a
>> (char *, len) pair. Change its callers to do likewise.  The LSMs
>> supporting this hook have had comments added to remind the developer
>> that there is more work to be done.
>>
>> The BPF security module provides all LSM hooks. While there has yet to
>> be a known instance of a BPF configuration that uses security contexts,
>> the possibility is real. In the existing implementation there is
>> potential for multiple frees in that case.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-integrity@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> Cc: audit@vger.kernel.org
>> Cc: netfilter-devel@vger.kernel.org
>> To: Pablo Neira Ayuso <pablo@netfilter.org>
>> Cc: linux-nfs@vger.kernel.org
>> Cc: Todd Kjos <tkjos@google.com>
>> ---
>>   drivers/android/binder.c                | 24 +++++++--------
>>   fs/ceph/xattr.c                         |  6 +++-
>>   fs/nfs/nfs4proc.c                       |  8 +++--
>>   fs/nfsd/nfs4xdr.c                       |  8 +++--
>>   include/linux/lsm_hook_defs.h           |  2 +-
>>   include/linux/security.h                | 35 ++++++++++++++++++++--
>>   include/net/scm.h                       | 11 +++----
>>   kernel/audit.c                          | 30 +++++++++----------
>>   kernel/auditsc.c                        | 23 +++++++-------
>>   net/ipv4/ip_sockglue.c                  | 10 +++----
>>   net/netfilter/nf_conntrack_netlink.c    | 10 +++----
>>   net/netfilter/nf_conntrack_standalone.c |  9 +++---
>>   net/netfilter/nfnetlink_queue.c         | 13 +++++---
>>   net/netlabel/netlabel_unlabeled.c       | 40 +++++++++++--------------
>>   net/netlabel/netlabel_user.c            | 11 ++++---
>>   security/apparmor/include/secid.h       |  2 +-
>>   security/apparmor/secid.c               | 11 +++++--
>>   security/security.c                     |  8 ++---
>>   security/selinux/hooks.c                | 11 +++++--
>>   19 files changed, 165 insertions(+), 107 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 978740537a1a..d4229bf6f789 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -3011,8 +3011,7 @@ static void binder_transaction(struct
>> binder_proc *proc,
>>       struct binder_context *context = proc->context;
>>       int t_debug_id = atomic_inc_return(&binder_last_id);
>>       ktime_t t_start_time = ktime_get();
>> -    char *secctx = NULL;
>> -    u32 secctx_sz = 0;
>> +    struct lsm_context lsmctx;
> Not initialized ?

Thank you for the review.
It makes sense to initialize it here. I will make the change.

>>       struct list_head sgc_head;
>>       struct list_head pf_head;
>>       const void __user *user_buffer = (const void __user *)
>> @@ -3291,7 +3290,8 @@ static void binder_transaction(struct
>> binder_proc *proc,
>>           size_t added_size;
>>             security_cred_getsecid(proc->cred, &secid);
>> -        ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
>> +        ret = security_secid_to_secctx(secid, &lsmctx.context,
>> +                           &lsmctx.len);
>>           if (ret) {
>>               binder_txn_error("%d:%d failed to get security context\n",
>>                   thread->pid, proc->pid);
>> @@ -3300,7 +3300,7 @@ static void binder_transaction(struct
>> binder_proc *proc,
>>               return_error_line = __LINE__;
>>               goto err_get_secctx_failed;
>>           }
>> -        added_size = ALIGN(secctx_sz, sizeof(u64));
>> +        added_size = ALIGN(lsmctx.len, sizeof(u64));
>>           extra_buffers_size += added_size;
>>           if (extra_buffers_size < added_size) {
>>               binder_txn_error("%d:%d integer overflow of
>> extra_buffers_size\n",
>> @@ -3334,23 +3334,23 @@ static void binder_transaction(struct
>> binder_proc *proc,
>>           t->buffer = NULL;
>>           goto err_binder_alloc_buf_failed;
>>       }
>> -    if (secctx) {
>> +    if (lsmctx.context) {
> From code inspection it is not immediately obvious. Can you
> guarantee that lsmctx is always initialized when the code
> gets to this point? Perhaps it is safer to just initialize when
> it is defined above (line 3014).
>
>

[PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Casey Schaufler]

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.

security_secid_to_secctx() and security_lsmproc_to_secctx()
will now return the length value on success instead of 0.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: netdev@vger.kernel.org
Cc: audit@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
Cc: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c                |  5 ++-
 include/linux/lsm_hook_defs.h           |  5 ++-
 include/linux/security.h                |  9 +++---
 include/net/scm.h                       |  5 ++-
 kernel/audit.c                          |  9 +++---
 kernel/auditsc.c                        | 16 ++++------
 net/ipv4/ip_sockglue.c                  |  4 +--
 net/netfilter/nf_conntrack_netlink.c    |  8 ++---
 net/netfilter/nf_conntrack_standalone.c |  4 +--
 net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
 net/netlabel/netlabel_unlabeled.c       | 14 +++------
 net/netlabel/netlabel_user.c            |  3 +-
 security/apparmor/include/secid.h       |  5 ++-
 security/apparmor/secid.c               | 26 +++++++--------
 security/security.c                     | 34 +++++++++-----------
 security/selinux/hooks.c                | 23 +++++++++++---
 security/smack/smack_lsm.c              | 42 +++++++++++++++----------
 17 files changed, 118 insertions(+), 121 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d4229bf6f789..5cec5b52bd4a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3290,9 +3290,8 @@ static void binder_transaction(struct binder_proc *proc,
 		size_t added_size;
 
 		security_cred_getsecid(proc->cred, &secid);
-		ret = security_secid_to_secctx(secid, &lsmctx.context,
-					       &lsmctx.len);
-		if (ret) {
+		ret = security_secid_to_secctx(secid, &lsmctx);
+		if (ret < 0) {
 			binder_txn_error("%d:%d failed to get security context\n",
 				thread->pid, proc->pid);
 			return_error = BR_FAILED_REPLY;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index c13df23132eb..01e5a8e09bba 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -295,10 +295,9 @@ LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
 	 char **value)
 LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
 LSM_HOOK(int, 0, ismaclabel, const char *name)
-LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
-	 u32 *seclen)
+LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, struct lsm_context *cp)
 LSM_HOOK(int, -EOPNOTSUPP, lsmprop_to_secctx, struct lsm_prop *prop,
-	 char **secdata, u32 *seclen)
+	 struct lsm_context *cp)
 LSM_HOOK(int, 0, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
 LSM_HOOK(void, LSM_RET_VOID, release_secctx, struct lsm_context *cp)
 LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1a3ca02224e8..64e8b18e6ea5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -584,8 +584,8 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
 int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
-int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
-int security_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata, u32 *seclen);
+int security_secid_to_secctx(u32 secid, struct lsm_context *cp);
+int security_lsmprop_to_secctx(struct lsm_prop *prop, struct lsm_context *cp);
 int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
 void security_release_secctx(struct lsm_context *cp);
 void security_inode_invalidate_secctx(struct inode *inode);
@@ -1557,14 +1557,13 @@ static inline int security_ismaclabel(const char *name)
 	return 0;
 }
 
-static inline int security_secid_to_secctx(u32 secid, char **secdata,
-					   u32 *seclen)
+static inline int security_secid_to_secctx(u32 secid, struct lsm_context *cp)
 {
 	return -EOPNOTSUPP;
 }
 
 static inline int security_lsmprop_to_secctx(struct lsm_prop *prop,
-					     char **secdata, u32 *seclen)
+					     struct lsm_context *cp)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/net/scm.h b/include/net/scm.h
index f75449e1d876..22bb49589fde 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -109,10 +109,9 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
 	int err;
 
 	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
-		err = security_secid_to_secctx(scm->secid, &ctx.context,
-					       &ctx.len);
+		err = security_secid_to_secctx(scm->secid, &ctx);
 
-		if (!err) {
+		if (err >= 0) {
 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, ctx.len,
 				 ctx.context);
 			security_release_secctx(&ctx);
diff --git a/kernel/audit.c b/kernel/audit.c
index bafd8fd2b1f3..5cdd9aeeb9bc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1473,9 +1473,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	case AUDIT_SIGNAL_INFO:
 		if (lsmprop_is_set(&audit_sig_lsm)) {
 			err = security_lsmprop_to_secctx(&audit_sig_lsm,
-							 &lsmctx.context,
-							 &lsmctx.len);
-			if (err)
+							 &lsmctx);
+			if (err < 0)
 				return err;
 		}
 		sig_data = kmalloc(struct_size(sig_data, ctx, lsmctx.len),
@@ -2188,8 +2187,8 @@ int audit_log_task_context(struct audit_buffer *ab)
 	if (!lsmprop_is_set(&prop))
 		return 0;
 
-	error = security_lsmprop_to_secctx(&prop, &ctx.context, &ctx.len);
-	if (error) {
+	error = security_lsmprop_to_secctx(&prop, &ctx);
+	if (error < 0) {
 		if (error != -EINVAL)
 			goto error_path;
 		return 0;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index c196dd4c9b54..4d3c22ba7149 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1109,7 +1109,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 			 from_kuid(&init_user_ns, auid),
 			 from_kuid(&init_user_ns, uid), sessionid);
 	if (lsmprop_is_set(prop)) {
-		if (security_lsmprop_to_secctx(prop, &ctx.context, &ctx.len)) {
+		if (security_lsmprop_to_secctx(prop, &ctx) < 0) {
 			audit_log_format(ab, " obj=(none)");
 			rc = 1;
 		} else {
@@ -1370,7 +1370,6 @@ static void audit_log_time(struct audit_context *context, struct audit_buffer **
 
 static void show_special(struct audit_context *context, int *call_panic)
 {
-	struct lsm_context lsmcxt;
 	struct audit_buffer *ab;
 	int i;
 
@@ -1393,16 +1392,14 @@ static void show_special(struct audit_context *context, int *call_panic)
 				 from_kgid(&init_user_ns, context->ipc.gid),
 				 context->ipc.mode);
 		if (lsmprop_is_set(&context->ipc.oprop)) {
-			char *ctx = NULL;
-			u32 len;
+			struct lsm_context lsmctx;
 
 			if (security_lsmprop_to_secctx(&context->ipc.oprop,
-						       &ctx, &len)) {
+						       &lsmctx) < 0) {
 				*call_panic = 1;
 			} else {
-				audit_log_format(ab, " obj=%s", ctx);
-				lsmcontext_init(&lsmcxt, ctx, len, 0);
-				security_release_secctx(&lsmcxt);
+				audit_log_format(ab, " obj=%s", lsmctx.context);
+				security_release_secctx(&lsmctx);
 			}
 		}
 		if (context->ipc.has_perm) {
@@ -1563,8 +1560,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
 	if (lsmprop_is_set(&n->oprop)) {
 		struct lsm_context ctx;
 
-		if (security_lsmprop_to_secctx(&n->oprop, &ctx.context,
-					       &ctx.len)) {
+		if (security_lsmprop_to_secctx(&n->oprop, &ctx) < 0) {
 			if (call_panic)
 				*call_panic = 2;
 		} else {
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index a8180dcc2a32..dadbf619b20f 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -136,8 +136,8 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
 	if (err)
 		return;
 
-	err = security_secid_to_secctx(secid, &ctx.context, &ctx.len);
-	if (err)
+	err = security_secid_to_secctx(secid, &ctx);
+	if (err < 0)
 		return;
 
 	put_cmsg(msg, SOL_IP, SCM_SECURITY, ctx.len, ctx.context);
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 86a57a3afdd6..dd74d4c67c69 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -360,8 +360,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
 	struct lsm_context ctx;
 	int ret;
 
-	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
-	if (ret)
+	ret = security_secid_to_secctx(ct->secmark, &ctx);
+	if (ret < 0)
 		return 0;
 
 	ret = -1;
@@ -665,8 +665,8 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
 #ifdef CONFIG_NF_CONNTRACK_SECMARK
 	int len, ret;
 
-	ret = security_secid_to_secctx(ct->secmark, NULL, &len);
-	if (ret)
+	ret = security_secid_to_secctx(ct->secmark, NULL);
+	if (ret < 0)
 		return 0;
 
 	return nla_total_size(0) /* CTA_SECCTX */
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 5f7fd23b7afe..502cf10aab41 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -175,8 +175,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
 	struct lsm_context ctx;
 	int ret;
 
-	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
-	if (ret)
+	ret = security_secid_to_secctx(ct->secmark, &ctx);
+	if (ret < 0)
 		return;
 
 	seq_printf(s, "secctx=%s ", ctx.context);
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 37757cd77cf1..5110f29b2f40 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -470,18 +470,18 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk)
 	return 0;
 }
 
-static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
+static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx)
 {
 	u32 seclen = 0;
 #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
+
 	if (!skb || !sk_fullsock(skb->sk))
 		return 0;
 
 	read_lock_bh(&skb->sk->sk_callback_lock);
 
 	if (skb->secmark)
-		security_secid_to_secctx(skb->secmark, secdata, &seclen);
-
+		seclen = security_secid_to_secctx(skb->secmark, ctx);
 	read_unlock_bh(&skb->sk->sk_callback_lock);
 #endif
 	return seclen;
@@ -567,8 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	enum ip_conntrack_info ctinfo = 0;
 	const struct nfnl_ct_hook *nfnl_ct;
 	bool csum_verify;
-	struct lsm_context scaff; /* scaffolding */
-	char *secdata = NULL;
+	struct lsm_context ctx;
 	u32 seclen = 0;
 	ktime_t tstamp;
 
@@ -643,8 +642,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	}
 
 	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
-		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
-		if (seclen)
+		seclen = nfqnl_get_sk_secctx(entskb, &ctx);
+		if (seclen >= 0)
 			size += nla_total_size(seclen);
 	}
 
@@ -783,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
-	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
+	if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
 		goto nla_put_failure;
 
 	if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
@@ -811,10 +810,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	}
 
 	nlh->nlmsg_len = skb->len;
-	if (seclen) {
-		lsmcontext_init(&scaff, secdata, seclen, 0);
-		security_release_secctx(&scaff);
-	}
+	if (seclen >= 0)
+		security_release_secctx(&ctx);
 	return skb;
 
 nla_put_failure:
@@ -822,10 +819,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	kfree_skb(skb);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 nlmsg_failure:
-	if (seclen) {
-		lsmcontext_init(&scaff, secdata, seclen, 0);
-		security_release_secctx(&scaff);
-	}
+	if (seclen >= 0)
+		security_release_secctx(&ctx);
 	return NULL;
 }
 
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 921fa8eeb451..bd7094f225d1 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -437,9 +437,7 @@ int netlbl_unlhsh_add(struct net *net,
 unlhsh_add_return:
 	rcu_read_unlock();
 	if (audit_buf != NULL) {
-		if (security_secid_to_secctx(secid,
-					     &ctx.context,
-					     &ctx.len) == 0) {
+		if (security_secid_to_secctx(secid, &ctx) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
 			security_release_secctx(&ctx);
 		}
@@ -492,8 +490,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
 					  addr->s_addr, mask->s_addr);
 		dev_put(dev);
 		if (entry != NULL &&
-		    security_secid_to_secctx(entry->secid,
-					     &ctx.context, &ctx.len) == 0) {
+		    security_secid_to_secctx(entry->secid, &ctx) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
 			security_release_secctx(&ctx);
 		}
@@ -551,8 +548,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
 					  addr, mask);
 		dev_put(dev);
 		if (entry != NULL &&
-		    security_secid_to_secctx(entry->secid,
-					     &ctx.context, &ctx.len) == 0) {
+		    security_secid_to_secctx(entry->secid, &ctx) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s", ctx.context);
 			security_release_secctx(&ctx);
 		}
@@ -1123,8 +1119,8 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
 		secid = addr6->secid;
 	}
 
-	ret_val = security_secid_to_secctx(secid, &ctx.context, &ctx.len);
-	if (ret_val != 0)
+	ret_val = security_secid_to_secctx(secid, &ctx);
+	if (ret_val < 0)
 		goto list_cb_failure;
 	ret_val = nla_put(cb_arg->skb,
 			  NLBL_UNLABEL_A_SECCTX,
diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
index f5e7a9919df1..0d04d23aafe7 100644
--- a/net/netlabel/netlabel_user.c
+++ b/net/netlabel/netlabel_user.c
@@ -98,8 +98,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
 			 audit_info->sessionid);
 
 	if (lsmprop_is_set(&audit_info->prop) &&
-	    security_lsmprop_to_secctx(&audit_info->prop, &ctx.context,
-				       &ctx.len) == 0) {
+	    security_lsmprop_to_secctx(&audit_info->prop, &ctx) > 0) {
 		audit_log_format(audit_buf, " subj=%s", ctx.context);
 		security_release_secctx(&ctx);
 	}
diff --git a/security/apparmor/include/secid.h b/security/apparmor/include/secid.h
index 8b92f90b6921..550a8d3ed527 100644
--- a/security/apparmor/include/secid.h
+++ b/security/apparmor/include/secid.h
@@ -25,9 +25,8 @@ struct aa_label;
 extern int apparmor_display_secid_mode;
 
 struct aa_label *aa_secid_to_label(u32 secid);
-int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
-int apparmor_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
-			       u32 *seclen);
+int apparmor_secid_to_secctx(u32 secid, struct lsm_context *cp);
+int apparmor_lsmprop_to_secctx(struct lsm_prop *prop, struct lsm_context *cp);
 int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
 void apparmor_release_secctx(struct lsm_context *cp);
 
diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
index 8d9ced8cdffd..5d92fc3ab8b4 100644
--- a/security/apparmor/secid.c
+++ b/security/apparmor/secid.c
@@ -61,23 +61,21 @@ struct aa_label *aa_secid_to_label(u32 secid)
 	return xa_load(&aa_secids, secid);
 }
 
-static int apparmor_label_to_secctx(struct aa_label *label, char **secdata,
-				    u32 *seclen)
+static int apparmor_label_to_secctx(struct aa_label *label,
+				    struct lsm_context *cp)
 {
 	/* TODO: cache secctx and ref count so we don't have to recreate */
 	int flags = FLAG_VIEW_SUBNS | FLAG_HIDDEN_UNCONFINED | FLAG_ABS_ROOT;
 	int len;
 
-	AA_BUG(!seclen);
-
 	if (!label)
 		return -EINVAL;
 
 	if (apparmor_display_secid_mode)
 		flags |= FLAG_SHOW_MODE;
 
-	if (secdata)
-		len = aa_label_asxprint(secdata, root_ns, label,
+	if (cp)
+		len = aa_label_asxprint(&cp->context, root_ns, label,
 					flags, GFP_ATOMIC);
 	else
 		len = aa_label_snxprint(NULL, 0, root_ns, label, flags);
@@ -85,26 +83,28 @@ static int apparmor_label_to_secctx(struct aa_label *label, char **secdata,
 	if (len < 0)
 		return -ENOMEM;
 
-	*seclen = len;
+	if (cp) {
+		cp->len = len;
+		cp->id = LSM_ID_APPARMOR;
+	}
 
-	return 0;
+	return len;
 }
 
-int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
+int apparmor_secid_to_secctx(u32 secid, struct lsm_context *cp)
 {
 	struct aa_label *label = aa_secid_to_label(secid);
 
-	return apparmor_label_to_secctx(label, secdata, seclen);
+	return apparmor_label_to_secctx(label, cp);
 }
 
-int apparmor_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
-			       u32 *seclen)
+int apparmor_lsmprop_to_secctx(struct lsm_prop *prop, struct lsm_context *cp)
 {
 	struct aa_label *label;
 
 	label = prop->apparmor.label;
 
-	return apparmor_label_to_secctx(label, secdata, seclen);
+	return apparmor_label_to_secctx(label, cp);
 }
 
 int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
diff --git a/security/security.c b/security/security.c
index 0c9c3a02704b..914d8c8beea7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4309,40 +4309,36 @@ EXPORT_SYMBOL(security_ismaclabel);
 /**
  * security_secid_to_secctx() - Convert a secid to a secctx
  * @secid: secid
- * @secdata: secctx
- * @seclen: secctx length
+ * @cp: the LSM context
  *
- * Convert secid to security context.  If @secdata is NULL the length of the
- * result will be returned in @seclen, but no @secdata will be returned.  This
+ * Convert secid to security context.  If @cp is NULL the length of the
+ * result will be returned, but no data will be returned.  This
  * does mean that the length could change between calls to check the length and
- * the next call which actually allocates and returns the @secdata.
+ * the next call which actually allocates and returns the data.
  *
- * Return: Return 0 on success, error on failure.
+ * Return: Return length of data on success, error on failure.
  */
-int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
+int security_secid_to_secctx(u32 secid, struct lsm_context *cp)
 {
-	return call_int_hook(secid_to_secctx, secid, secdata, seclen);
+	return call_int_hook(secid_to_secctx, secid, cp);
 }
 EXPORT_SYMBOL(security_secid_to_secctx);
 
 /**
  * security_lsmprop_to_secctx() - Convert a lsm_prop to a secctx
  * @prop: lsm specific information
- * @secdata: secctx
- * @seclen: secctx length
+ * @cp: the LSM context
  *
- * Convert a @prop entry to security context.  If @secdata is NULL the
- * length of the result will be returned in @seclen, but no @secdata
- * will be returned.  This does mean that the length could change between
- * calls to check the length and the next call which actually allocates
- * and returns the @secdata.
+ * Convert a @prop entry to security context.  If @cp is NULL the
+ * length of the result will be returned. This does mean that the
+ * length could change between calls to check the length and the
+ * next call which actually allocates and returns the @cp.
  *
- * Return: Return 0 on success, error on failure.
+ * Return: Return length of data on success, error on failure.
  */
-int security_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
-			       u32 *seclen)
+int security_lsmprop_to_secctx(struct lsm_prop *prop, struct lsm_context *cp)
 {
-	return call_int_hook(lsmprop_to_secctx, prop, secdata, seclen);
+	return call_int_hook(lsmprop_to_secctx, prop, cp);
 }
 EXPORT_SYMBOL(security_lsmprop_to_secctx);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1503d398c87d..692735eb04aa 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6607,15 +6607,28 @@ static int selinux_ismaclabel(const char *name)
 	return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
 }
 
-static int selinux_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
+static int selinux_secid_to_secctx(u32 secid, struct lsm_context *cp)
 {
-	return security_sid_to_context(secid, secdata, seclen);
+	u32 seclen;
+	u32 ret;
+
+	if (cp) {
+		cp->id = LSM_ID_SELINUX;
+		ret = security_sid_to_context(secid, &cp->context, &cp->len);
+		if (ret < 0)
+			return ret;
+		return cp->len;
+	}
+	ret = security_sid_to_context(secid, NULL, &seclen);
+	if (ret < 0)
+		return ret;
+	return seclen;
 }
 
-static int selinux_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
-				     u32 *seclen)
+static int selinux_lsmprop_to_secctx(struct lsm_prop *prop,
+				     struct lsm_context *cp)
 {
-	return selinux_secid_to_secctx(prop->selinux.secid, secdata, seclen);
+	return selinux_secid_to_secctx(prop->selinux.secid, cp);
 }
 
 static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0c476282e279..d52163d3dd64 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4817,22 +4817,35 @@ static int smack_ismaclabel(const char *name)
 	return (strcmp(name, XATTR_SMACK_SUFFIX) == 0);
 }
 
+/**
+ * smack_to_secctx - fill a lsm_context
+ * @skp: Smack label
+ * @cp: destination
+ *
+ * Fill the passed @cp and return the length of the string
+ */
+static int smack_to_secctx(struct smack_known *skp, struct lsm_context *cp)
+{
+	int len = strlen(skp->smk_known);
+
+	if (cp) {
+		cp->context = skp->smk_known;
+		cp->len = len;
+		cp->id = LSM_ID_SMACK;
+	}
+	return len;
+}
+
 /**
  * smack_secid_to_secctx - return the smack label for a secid
  * @secid: incoming integer
- * @secdata: destination
- * @seclen: how long it is
+ * @cp: destination
  *
  * Exists for networking code.
  */
-static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
+static int smack_secid_to_secctx(u32 secid, struct lsm_context *cp)
 {
-	struct smack_known *skp = smack_from_secid(secid);
-
-	if (secdata)
-		*secdata = skp->smk_known;
-	*seclen = strlen(skp->smk_known);
-	return 0;
+	return smack_to_secctx(smack_from_secid(secid), cp);
 }
 
 /**
@@ -4843,15 +4856,10 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
  *
  * Exists for audit code.
  */
-static int smack_lsmprop_to_secctx(struct lsm_prop *prop, char **secdata,
-				   u32 *seclen)
+static int smack_lsmprop_to_secctx(struct lsm_prop *prop,
+				   struct lsm_context *cp)
 {
-	struct smack_known *skp = prop->smack.skp;
-
-	if (secdata)
-		*secdata = skp->smk_known;
-	*seclen = strlen(skp->smk_known);
-	return 0;
+	return smack_to_secctx(prop->smack.skp, cp);
 }
 
 /**
-- 
2.46.0

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Pablo Neira Ayuso]

Hi Casey,

This is a review of the netfilter chunk.

On Wed, Oct 23, 2024 at 02:21:55PM -0700, Casey Schaufler wrote:
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 86a57a3afdd6..dd74d4c67c69 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -360,8 +360,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>  	struct lsm_context ctx;
>  	int ret;
>  
> -	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
> -	if (ret)
> +	ret = security_secid_to_secctx(ct->secmark, &ctx);
> +	if (ret < 0)
>  		return 0;
>  
>  	ret = -1;
> @@ -665,8 +665,8 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
>  #ifdef CONFIG_NF_CONNTRACK_SECMARK
>  	int len, ret;
>  
> -	ret = security_secid_to_secctx(ct->secmark, NULL, &len);
> -	if (ret)
> +	ret = security_secid_to_secctx(ct->secmark, NULL);

This breaks here.

len is really used, this should be instead:

	ret = security_secid_to_secctx(ct->secmark, &ctx);

[...]
        return nla_total_size(0) /* CTA_SECCTX */
               + nla_total_size(sizeof(char) * ctx.len); /* CTA_SECCTX_NAME */
#else
        return 0;
#endif
}

> +	if (ret < 0)
>  		return 0;
>  
>  	return nla_total_size(0) /* CTA_SECCTX */
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 5f7fd23b7afe..502cf10aab41 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -175,8 +175,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>  	struct lsm_context ctx;
>  	int ret;
>  
> -	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
> -	if (ret)
> +	ret = security_secid_to_secctx(ct->secmark, &ctx);
> +	if (ret < 0)
>  		return;
>  
>  	seq_printf(s, "secctx=%s ", ctx.context);
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 37757cd77cf1..5110f29b2f40 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -470,18 +470,18 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk)
>  	return 0;
>  }
>  
> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx)
>  {
>  	u32 seclen = 0;
>  #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> +

remove unneeded line.

>  	if (!skb || !sk_fullsock(skb->sk))
>  		return 0;
>  
>  	read_lock_bh(&skb->sk->sk_callback_lock);
>  
>  	if (skb->secmark)
> -		security_secid_to_secctx(skb->secmark, secdata, &seclen);
> -
> +		seclen = security_secid_to_secctx(skb->secmark, ctx);
>  	read_unlock_bh(&skb->sk->sk_callback_lock);
>  #endif
>  	return seclen;
> @@ -567,8 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	enum ip_conntrack_info ctinfo = 0;
>  	const struct nfnl_ct_hook *nfnl_ct;
>  	bool csum_verify;
> -	struct lsm_context scaff; /* scaffolding */
> -	char *secdata = NULL;
> +	struct lsm_context ctx;

Help us make this get closer to revert xmas tree:

  	enum ip_conntrack_info ctinfo = 0;
  	const struct nfnl_ct_hook *nfnl_ct;
+	struct lsm_context ctx;
  	bool csum_verify;
-	struct lsm_context scaff; /* scaffolding */
-	char *secdata = NULL;

>  	bool csum_verify;
> -	struct lsm_context scaff; /* scaffolding */
> -	char *secdata = NULL;

>  	u32 seclen = 0;
>  	ktime_t tstamp;
>  
> @@ -643,8 +642,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	}
>  
>  	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> -		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
> -		if (seclen)
> +		seclen = nfqnl_get_sk_secctx(entskb, &ctx);
> +		if (seclen >= 0)
>  			size += nla_total_size(seclen);
>  	}
>  
> @@ -783,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
>  		goto nla_put_failure;
>  
> -	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> +	if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
>  		goto nla_put_failure;
>  
>  	if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> @@ -811,10 +810,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	}
>  
>  	nlh->nlmsg_len = skb->len;
> -	if (seclen) {
> -		lsmcontext_init(&scaff, secdata, seclen, 0);
> -		security_release_secctx(&scaff);
> -	}
> +	if (seclen >= 0)
> +		security_release_secctx(&ctx);
>  	return skb;
>  
>  nla_put_failure:
> @@ -822,10 +819,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	kfree_skb(skb);
>  	net_err_ratelimited("nf_queue: error creating packet message\n");
>  nlmsg_failure:
> -	if (seclen) {
> -		lsmcontext_init(&scaff, secdata, seclen, 0);
> -		security_release_secctx(&scaff);
> -	}
> +	if (seclen >= 0)
> +		security_release_secctx(&ctx);
>  	return NULL;
>  }
>  

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Casey Schaufler]

On 10/24/2024 9:10 AM, Pablo Neira Ayuso wrote:
> Hi Casey,
>
> This is a review of the netfilter chunk.

Thank you.

> On Wed, Oct 23, 2024 at 02:21:55PM -0700, Casey Schaufler wrote:
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index 86a57a3afdd6..dd74d4c67c69 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -360,8 +360,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>>  	struct lsm_context ctx;
>>  	int ret;
>>  
>> -	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
>> -	if (ret)
>> +	ret = security_secid_to_secctx(ct->secmark, &ctx);
>> +	if (ret < 0)
>>  		return 0;
>>  
>>  	ret = -1;
>> @@ -665,8 +665,8 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
>>  #ifdef CONFIG_NF_CONNTRACK_SECMARK
>>  	int len, ret;
>>  
>> -	ret = security_secid_to_secctx(ct->secmark, NULL, &len);
>> -	if (ret)
>> +	ret = security_secid_to_secctx(ct->secmark, NULL);
> This breaks here.
>
> len is really used, this should be instead:
>
> 	ret = security_secid_to_secctx(ct->secmark, &ctx);
>
> [...]
>         return nla_total_size(0) /* CTA_SECCTX */
>                + nla_total_size(sizeof(char) * ctx.len); /* CTA_SECCTX_NAME */
> #else
>         return 0;
> #endif
> }

I'll fix that.

>> +	if (ret < 0)
>>  		return 0;
>>  
>>  	return nla_total_size(0) /* CTA_SECCTX */
>> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
>> index 5f7fd23b7afe..502cf10aab41 100644
>> --- a/net/netfilter/nf_conntrack_standalone.c
>> +++ b/net/netfilter/nf_conntrack_standalone.c
>> @@ -175,8 +175,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>>  	struct lsm_context ctx;
>>  	int ret;
>>  
>> -	ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
>> -	if (ret)
>> +	ret = security_secid_to_secctx(ct->secmark, &ctx);
>> +	if (ret < 0)
>>  		return;
>>  
>>  	seq_printf(s, "secctx=%s ", ctx.context);
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 37757cd77cf1..5110f29b2f40 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -470,18 +470,18 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk)
>>  	return 0;
>>  }
>>  
>> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx)
>>  {
>>  	u32 seclen = 0;
>>  #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
>> +
> remove unneeded line.

Will do.

>>  	if (!skb || !sk_fullsock(skb->sk))
>>  		return 0;
>>  
>>  	read_lock_bh(&skb->sk->sk_callback_lock);
>>  
>>  	if (skb->secmark)
>> -		security_secid_to_secctx(skb->secmark, secdata, &seclen);
>> -
>> +		seclen = security_secid_to_secctx(skb->secmark, ctx);
>>  	read_unlock_bh(&skb->sk->sk_callback_lock);
>>  #endif
>>  	return seclen;
>> @@ -567,8 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	enum ip_conntrack_info ctinfo = 0;
>>  	const struct nfnl_ct_hook *nfnl_ct;
>>  	bool csum_verify;
>> -	struct lsm_context scaff; /* scaffolding */
>> -	char *secdata = NULL;
>> +	struct lsm_context ctx;
> Help us make this get closer to revert xmas tree:
>
>   	enum ip_conntrack_info ctinfo = 0;
>   	const struct nfnl_ct_hook *nfnl_ct;
> +	struct lsm_context ctx;
>   	bool csum_verify;
> -	struct lsm_context scaff; /* scaffolding */
> -	char *secdata = NULL;

Will do.

>>  	bool csum_verify;
>> -	struct lsm_context scaff; /* scaffolding */
>> -	char *secdata = NULL;
>>  	u32 seclen = 0;
>>  	ktime_t tstamp;
>>  
>> @@ -643,8 +642,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	}
>>  
>>  	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
>> -		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
>> -		if (seclen)
>> +		seclen = nfqnl_get_sk_secctx(entskb, &ctx);
>> +		if (seclen >= 0)
>>  			size += nla_total_size(seclen);
>>  	}
>>  
>> @@ -783,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
>>  		goto nla_put_failure;
>>  
>> -	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
>> +	if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
>>  		goto nla_put_failure;
>>  
>>  	if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
>> @@ -811,10 +810,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	}
>>  
>>  	nlh->nlmsg_len = skb->len;
>> -	if (seclen) {
>> -		lsmcontext_init(&scaff, secdata, seclen, 0);
>> -		security_release_secctx(&scaff);
>> -	}
>> +	if (seclen >= 0)
>> +		security_release_secctx(&ctx);
>>  	return skb;
>>  
>>  nla_put_failure:
>> @@ -822,10 +819,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	kfree_skb(skb);
>>  	net_err_ratelimited("nf_queue: error creating packet message\n");
>>  nlmsg_failure:
>> -	if (seclen) {
>> -		lsmcontext_init(&scaff, secdata, seclen, 0);
>> -		security_release_secctx(&scaff);
>> -	}
>> +	if (seclen >= 0)
>> +		security_release_secctx(&ctx);
>>  	return NULL;
>>  }
>>  

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Paul Moore]

On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> 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.
> 
> security_secid_to_secctx() and security_lsmproc_to_secctx()
> will now return the length value on success instead of 0.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: netdev@vger.kernel.org
> Cc: audit@vger.kernel.org
> Cc: netfilter-devel@vger.kernel.org
> Cc: Todd Kjos <tkjos@google.com>
> ---
>  drivers/android/binder.c                |  5 ++-
>  include/linux/lsm_hook_defs.h           |  5 ++-
>  include/linux/security.h                |  9 +++---
>  include/net/scm.h                       |  5 ++-
>  kernel/audit.c                          |  9 +++---
>  kernel/auditsc.c                        | 16 ++++------
>  net/ipv4/ip_sockglue.c                  |  4 +--
>  net/netfilter/nf_conntrack_netlink.c    |  8 ++---
>  net/netfilter/nf_conntrack_standalone.c |  4 +--
>  net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
>  net/netlabel/netlabel_unlabeled.c       | 14 +++------
>  net/netlabel/netlabel_user.c            |  3 +-
>  security/apparmor/include/secid.h       |  5 ++-
>  security/apparmor/secid.c               | 26 +++++++--------
>  security/security.c                     | 34 +++++++++-----------
>  security/selinux/hooks.c                | 23 +++++++++++---
>  security/smack/smack_lsm.c              | 42 +++++++++++++++----------
>  17 files changed, 118 insertions(+), 121 deletions(-)

See my note on patch 1/5, merging into lsm/dev.

--
paul-moore.com

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Pablo Neira Ayuso]

Hi Paul,

This patch breaks nf_conntrack_netlink, Casey mentioned that he will
post another series.

On Thu, Oct 31, 2024 at 06:53:38PM -0400, Paul Moore wrote:
> On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> > 
> > 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.
> > 
> > security_secid_to_secctx() and security_lsmproc_to_secctx()
> > will now return the length value on success instead of 0.
> > 
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > Cc: netdev@vger.kernel.org
> > Cc: audit@vger.kernel.org
> > Cc: netfilter-devel@vger.kernel.org
> > Cc: Todd Kjos <tkjos@google.com>
> > ---
> >  drivers/android/binder.c                |  5 ++-
> >  include/linux/lsm_hook_defs.h           |  5 ++-
> >  include/linux/security.h                |  9 +++---
> >  include/net/scm.h                       |  5 ++-
> >  kernel/audit.c                          |  9 +++---
> >  kernel/auditsc.c                        | 16 ++++------
> >  net/ipv4/ip_sockglue.c                  |  4 +--
> >  net/netfilter/nf_conntrack_netlink.c    |  8 ++---
> >  net/netfilter/nf_conntrack_standalone.c |  4 +--
> >  net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
> >  net/netlabel/netlabel_unlabeled.c       | 14 +++------
> >  net/netlabel/netlabel_user.c            |  3 +-
> >  security/apparmor/include/secid.h       |  5 ++-
> >  security/apparmor/secid.c               | 26 +++++++--------
> >  security/security.c                     | 34 +++++++++-----------
> >  security/selinux/hooks.c                | 23 +++++++++++---
> >  security/smack/smack_lsm.c              | 42 +++++++++++++++----------
> >  17 files changed, 118 insertions(+), 121 deletions(-)
> 
> See my note on patch 1/5, merging into lsm/dev.

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Pablo Neira Ayuso]

On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
> Hi Paul,
> 
> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
> post another series.

Please, see:

https://lore.kernel.org/netfilter-devel/ZxpxZuErvXSLApsf@calendula/

> On Thu, Oct 31, 2024 at 06:53:38PM -0400, Paul Moore wrote:
> > On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > 
> > > 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.
> > > 
> > > security_secid_to_secctx() and security_lsmproc_to_secctx()
> > > will now return the length value on success instead of 0.
> > > 
> > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > > Cc: netdev@vger.kernel.org
> > > Cc: audit@vger.kernel.org
> > > Cc: netfilter-devel@vger.kernel.org
> > > Cc: Todd Kjos <tkjos@google.com>
> > > ---
> > >  drivers/android/binder.c                |  5 ++-
> > >  include/linux/lsm_hook_defs.h           |  5 ++-
> > >  include/linux/security.h                |  9 +++---
> > >  include/net/scm.h                       |  5 ++-
> > >  kernel/audit.c                          |  9 +++---
> > >  kernel/auditsc.c                        | 16 ++++------
> > >  net/ipv4/ip_sockglue.c                  |  4 +--
> > >  net/netfilter/nf_conntrack_netlink.c    |  8 ++---
> > >  net/netfilter/nf_conntrack_standalone.c |  4 +--
> > >  net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
> > >  net/netlabel/netlabel_unlabeled.c       | 14 +++------
> > >  net/netlabel/netlabel_user.c            |  3 +-
> > >  security/apparmor/include/secid.h       |  5 ++-
> > >  security/apparmor/secid.c               | 26 +++++++--------
> > >  security/security.c                     | 34 +++++++++-----------
> > >  security/selinux/hooks.c                | 23 +++++++++++---
> > >  security/smack/smack_lsm.c              | 42 +++++++++++++++----------
> > >  17 files changed, 118 insertions(+), 121 deletions(-)
> > 
> > See my note on patch 1/5, merging into lsm/dev.
> 

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Casey Schaufler]

On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
>> Hi Paul,
>>
>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
>> post another series.

I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
Or, if you want to fix it yourself, in ctnetlink_secctx_size() remove the
declaration of "len" and replace its use in the return with "ret".

> Please, see:
>
> https://lore.kernel.org/netfilter-devel/ZxpxZuErvXSLApsf@calendula/
>
>> On Thu, Oct 31, 2024 at 06:53:38PM -0400, Paul Moore wrote:
>>> On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> 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.
>>>>
>>>> security_secid_to_secctx() and security_lsmproc_to_secctx()
>>>> will now return the length value on success instead of 0.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> Cc: netdev@vger.kernel.org
>>>> Cc: audit@vger.kernel.org
>>>> Cc: netfilter-devel@vger.kernel.org
>>>> Cc: Todd Kjos <tkjos@google.com>
>>>> ---
>>>>  drivers/android/binder.c                |  5 ++-
>>>>  include/linux/lsm_hook_defs.h           |  5 ++-
>>>>  include/linux/security.h                |  9 +++---
>>>>  include/net/scm.h                       |  5 ++-
>>>>  kernel/audit.c                          |  9 +++---
>>>>  kernel/auditsc.c                        | 16 ++++------
>>>>  net/ipv4/ip_sockglue.c                  |  4 +--
>>>>  net/netfilter/nf_conntrack_netlink.c    |  8 ++---
>>>>  net/netfilter/nf_conntrack_standalone.c |  4 +--
>>>>  net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
>>>>  net/netlabel/netlabel_unlabeled.c       | 14 +++------
>>>>  net/netlabel/netlabel_user.c            |  3 +-
>>>>  security/apparmor/include/secid.h       |  5 ++-
>>>>  security/apparmor/secid.c               | 26 +++++++--------
>>>>  security/security.c                     | 34 +++++++++-----------
>>>>  security/selinux/hooks.c                | 23 +++++++++++---
>>>>  security/smack/smack_lsm.c              | 42 +++++++++++++++----------
>>>>  17 files changed, 118 insertions(+), 121 deletions(-)
>>> See my note on patch 1/5, merging into lsm/dev.

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Pablo Neira Ayuso]

On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
> > On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
> >> Hi Paul,
> >>
> >> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
> >> post another series.
> 
> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?

No idea. I don't know what is the status of this series. I would
suggest to repost a new series.

> Or, if you want to fix it yourself, in ctnetlink_secctx_size() remove the
> declaration of "len" and replace its use in the return with "ret".

No, sorry, I won't do that.

Thanks.

> > Please, see:
> >
> > https://lore.kernel.org/netfilter-devel/ZxpxZuErvXSLApsf@calendula/
> >
> >> On Thu, Oct 31, 2024 at 06:53:38PM -0400, Paul Moore wrote:
> >>> On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> 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.
> >>>>
> >>>> security_secid_to_secctx() and security_lsmproc_to_secctx()
> >>>> will now return the length value on success instead of 0.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>> Cc: netdev@vger.kernel.org
> >>>> Cc: audit@vger.kernel.org
> >>>> Cc: netfilter-devel@vger.kernel.org
> >>>> Cc: Todd Kjos <tkjos@google.com>
> >>>> ---
> >>>>  drivers/android/binder.c                |  5 ++-
> >>>>  include/linux/lsm_hook_defs.h           |  5 ++-
> >>>>  include/linux/security.h                |  9 +++---
> >>>>  include/net/scm.h                       |  5 ++-
> >>>>  kernel/audit.c                          |  9 +++---
> >>>>  kernel/auditsc.c                        | 16 ++++------
> >>>>  net/ipv4/ip_sockglue.c                  |  4 +--
> >>>>  net/netfilter/nf_conntrack_netlink.c    |  8 ++---
> >>>>  net/netfilter/nf_conntrack_standalone.c |  4 +--
> >>>>  net/netfilter/nfnetlink_queue.c         | 27 +++++++---------
> >>>>  net/netlabel/netlabel_unlabeled.c       | 14 +++------
> >>>>  net/netlabel/netlabel_user.c            |  3 +-
> >>>>  security/apparmor/include/secid.h       |  5 ++-
> >>>>  security/apparmor/secid.c               | 26 +++++++--------
> >>>>  security/security.c                     | 34 +++++++++-----------
> >>>>  security/selinux/hooks.c                | 23 +++++++++++---
> >>>>  security/smack/smack_lsm.c              | 42 +++++++++++++++----------
> >>>>  17 files changed, 118 insertions(+), 121 deletions(-)
> >>> See my note on patch 1/5, merging into lsm/dev.

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Casey Schaufler]

On 11/1/2024 12:25 AM, Pablo Neira Ayuso wrote:
> On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
>> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
>>> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
>>>> Hi Paul,
>>>>
>>>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
>>>> post another series.
>> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
> No idea. I don't know what is the status of this series. I would
> suggest to repost a new series.

I will post v4 shortly. Thanks for the feedback.

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Paul Moore]

On Fri, Nov 1, 2024 at 12:14 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 11/1/2024 12:25 AM, Pablo Neira Ayuso wrote:
> > On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
> >> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
> >>> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
> >>>> Hi Paul,
> >>>>
> >>>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
> >>>> post another series.
> >> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
> > No idea. I don't know what is the status of this series. I would
> > suggest to repost a new series.
>
> I will post v4 shortly. Thanks for the feedback.

Please just post a fix against v2 using lsm/dev as a base.

-- 
paul-moore.com

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Paul Moore]

On Fri, Nov 1, 2024 at 12:35 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Nov 1, 2024 at 12:14 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 11/1/2024 12:25 AM, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
> > >> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
> > >>> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
> > >>>> Hi Paul,
> > >>>>
> > >>>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
> > >>>> post another series.
> > >> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
> > > No idea. I don't know what is the status of this series. I would
> > > suggest to repost a new series.
> >
> > I will post v4 shortly. Thanks for the feedback.
>
> Please just post a fix against v2 using lsm/dev as a base.

That should have been "against *v3* using lsm/dev as a base".

Also, since I didn't explicitly mention it, if I don't see a fix by
dinner time tonight (US East Coast), I'll revert this patchset, but
I'd like to avoid that if possible.

Sorry for the screw-up on my end folks, I was trying to get things
done before Halloween kicked off and it looks like I was a bit too
hasty in my review/merging.

-- 
paul-moore.com

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Casey Schaufler]

On 11/1/2024 9:42 AM, Paul Moore wrote:
> On Fri, Nov 1, 2024 at 12:35 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Fri, Nov 1, 2024 at 12:14 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 11/1/2024 12:25 AM, Pablo Neira Ayuso wrote:
>>>> On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
>>>>> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
>>>>>> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
>>>>>>> Hi Paul,
>>>>>>>
>>>>>>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
>>>>>>> post another series.
>>>>> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
>>>> No idea. I don't know what is the status of this series. I would
>>>> suggest to repost a new series.
>>> I will post v4 shortly. Thanks for the feedback.
>> Please just post a fix against v2 using lsm/dev as a base.
> That should have been "against *v3* using lsm/dev as a base".
>
> Also, since I didn't explicitly mention it, if I don't see a fix by
> dinner time tonight (US East Coast), I'll revert this patchset, but
> I'd like to avoid that if possible.

I will have this as quickly as I can. The patch is easy, but the overhead
may slow it down a bit. I should have it in time to avoid the revert.


> Sorry for the screw-up on my end folks, I was trying to get things
> done before Halloween kicked off and it looks like I was a bit too
> hasty in my review/merging.
>

Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Paul Moore]

On Fri, Nov 1, 2024 at 12:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 11/1/2024 9:42 AM, Paul Moore wrote:
> > On Fri, Nov 1, 2024 at 12:35 PM Paul Moore <paul@paul-moore.com> wrote:
> >> On Fri, Nov 1, 2024 at 12:14 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 11/1/2024 12:25 AM, Pablo Neira Ayuso wrote:
> >>>> On Thu, Oct 31, 2024 at 04:58:13PM -0700, Casey Schaufler wrote:
> >>>>> On 10/31/2024 4:23 PM, Pablo Neira Ayuso wrote:
> >>>>>> On Fri, Nov 01, 2024 at 12:15:16AM +0100, Pablo Neira Ayuso wrote:
> >>>>>>> Hi Paul,
> >>>>>>>
> >>>>>>> This patch breaks nf_conntrack_netlink, Casey mentioned that he will
> >>>>>>> post another series.
> >>>>> I have a fix, it is pretty simple. How about I send a 6/5 patch for it?
> >>>> No idea. I don't know what is the status of this series. I would
> >>>> suggest to repost a new series.
> >>> I will post v4 shortly. Thanks for the feedback.
> >> Please just post a fix against v2 using lsm/dev as a base.
> > That should have been "against *v3* using lsm/dev as a base".
> >
> > Also, since I didn't explicitly mention it, if I don't see a fix by
> > dinner time tonight (US East Coast), I'll revert this patchset, but
> > I'd like to avoid that if possible.
>
> I will have this as quickly as I can. The patch is easy, but the overhead
> may slow it down a bit. I should have it in time to avoid the revert.

It turns out there is no rush on this as it looks like the Rust
bindings are going to be the one that ends up pushing this out past
the next merge window as there is a conflict with changes to the Rust
LSM helpers in the VFS tree.

We still obviously need to the fix, so please keep going with the fix
based against v3; I'm going to move the v3 patchset from lsm/dev to
lsm/dev-staging, this will still allow for the usual LSM testing but
will shield it from linux-next.

-- 
paul-moore.com

[PATCH v3 3/5] LSM: Use lsm_context in security_inode_getsecctx

[Casey Schaufler]

Change the security_inode_getsecctx() interface to fill a lsm_context
structure instead of data and length pointers.  This provides
the information about which LSM created the context so that
security_release_secctx() can use the correct hook.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-nfs@vger.kernel.org
---
 fs/nfsd/nfs4xdr.c             | 26 ++++++++++----------------
 include/linux/lsm_hook_defs.h |  4 ++--
 include/linux/security.h      |  5 +++--
 security/security.c           | 12 ++++++------
 security/selinux/hooks.c      | 10 ++++++----
 security/smack/smack_lsm.c    |  7 ++++---
 6 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 537ad363d70a..93faa238b979 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2827,11 +2827,11 @@ static __be32 nfsd4_encode_nfsace4(struct xdr_stream *xdr, struct svc_rqst *rqst
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 static inline __be32
 nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
-			    void *context, int len)
+			    const struct lsm_context *context)
 {
 	__be32 *p;
 
-	p = xdr_reserve_space(xdr, len + 4 + 4 + 4);
+	p = xdr_reserve_space(xdr, context->len + 4 + 4 + 4);
 	if (!p)
 		return nfserr_resource;
 
@@ -2841,13 +2841,13 @@ nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
 	 */
 	*p++ = cpu_to_be32(0); /* lfs */
 	*p++ = cpu_to_be32(0); /* pi */
-	p = xdr_encode_opaque(p, context, len);
+	p = xdr_encode_opaque(p, context->context, context->len);
 	return 0;
 }
 #else
 static inline __be32
 nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
-			    void *context, int len)
+			    struct lsm_context *context)
 { return 0; }
 #endif
 
@@ -2930,8 +2930,7 @@ struct nfsd4_fattr_args {
 	struct nfs4_acl		*acl;
 	u64			size;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	void			*context;
-	int			contextlen;
+	struct lsm_context	context;
 #endif
 	u32			rdattr_err;
 	bool			contextsupport;
@@ -3386,8 +3385,7 @@ static __be32 nfsd4_encode_fattr4_suppattr_exclcreat(struct xdr_stream *xdr,
 static __be32 nfsd4_encode_fattr4_sec_label(struct xdr_stream *xdr,
 					    const struct nfsd4_fattr_args *args)
 {
-	return nfsd4_encode_security_label(xdr, args->rqstp,
-					   args->context, args->contextlen);
+	return nfsd4_encode_security_label(xdr, args->rqstp, &args->context);
 }
 #endif
 
@@ -3538,7 +3536,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	args.ignore_crossmnt = (ignore_crossmnt != 0);
 	args.acl = NULL;
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	args.context = NULL;
+	args.context.context = NULL;
 #endif
 
 	/*
@@ -3616,7 +3614,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	     attrmask[0] & FATTR4_WORD0_SUPPORTED_ATTRS) {
 		if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
 			err = security_inode_getsecctx(d_inode(dentry),
-						&args.context, &args.contextlen);
+						&args.context);
 		else
 			err = -EOPNOTSUPP;
 		args.contextsupport = (err == 0);
@@ -3653,12 +3651,8 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 
 out:
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-	if (args.context) {
-		struct lsm_context scaff; /* scaffolding */
-
-		lsmcontext_init(&scaff, args.context, args.contextlen, 0);
-		security_release_secctx(&scaff);
-	}
+	if (args.context.context)
+		security_release_secctx(&args.context);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 	kfree(args.acl);
 	if (tempfh) {
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 01e5a8e09bba..69e1076448c6 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -303,8 +303,8 @@ LSM_HOOK(void, LSM_RET_VOID, release_secctx, struct lsm_context *cp)
 LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
 LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
 LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
-LSM_HOOK(int, -EOPNOTSUPP, inode_getsecctx, struct inode *inode, void **ctx,
-	 u32 *ctxlen)
+LSM_HOOK(int, -EOPNOTSUPP, inode_getsecctx, struct inode *inode,
+	 struct lsm_context *cp)
 
 #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
 LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
diff --git a/include/linux/security.h b/include/linux/security.h
index 64e8b18e6ea5..7d0adc1833ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -591,7 +591,7 @@ void security_release_secctx(struct lsm_context *cp);
 void security_inode_invalidate_secctx(struct inode *inode);
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
-int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
+int security_inode_getsecctx(struct inode *inode, struct lsm_context *cp);
 int security_locked_down(enum lockdown_reason what);
 int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
 		      void *val, size_t val_len, u64 id, u64 flags);
@@ -1591,7 +1591,8 @@ static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32
 {
 	return -EOPNOTSUPP;
 }
-static inline int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
+static inline int security_inode_getsecctx(struct inode *inode,
+					   struct lsm_context *cp)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/security.c b/security/security.c
index 914d8c8beea7..4ca3c9e28b6f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4431,17 +4431,17 @@ EXPORT_SYMBOL(security_inode_setsecctx);
 /**
  * security_inode_getsecctx() - Get the security label of an inode
  * @inode: inode
- * @ctx: secctx
- * @ctxlen: length of secctx
+ * @cp: security context
  *
- * On success, returns 0 and fills out @ctx and @ctxlen with the security
- * context for the given @inode.
+ * On success, returns 0 and fills out @cp with the security context
+ * for the given @inode.
  *
  * Return: Returns 0 on success, error on failure.
  */
-int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
+int security_inode_getsecctx(struct inode *inode, struct lsm_context *cp)
 {
-	return call_int_hook(inode_getsecctx, inode, ctx, ctxlen);
+	memset(cp, 0, sizeof(*cp));
+	return call_int_hook(inode_getsecctx, inode, cp);
 }
 EXPORT_SYMBOL(security_inode_getsecctx);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 692735eb04aa..ce5e45abd8d3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6678,14 +6678,16 @@ static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 				     ctx, ctxlen, 0, NULL);
 }
 
-static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
+static int selinux_inode_getsecctx(struct inode *inode, struct lsm_context *cp)
 {
-	int len = 0;
+	int len;
 	len = selinux_inode_getsecurity(&nop_mnt_idmap, inode,
-					XATTR_SELINUX_SUFFIX, ctx, true);
+					XATTR_SELINUX_SUFFIX,
+					(void **)&cp->context, true);
 	if (len < 0)
 		return len;
-	*ctxlen = len;
+	cp->len = len;
+	cp->id = LSM_ID_SELINUX;
 	return 0;
 }
 #ifdef CONFIG_KEYS
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d52163d3dd64..c9ec4d93fb13 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4899,12 +4899,13 @@ static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 				     ctx, ctxlen, 0, NULL);
 }
 
-static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
+static int smack_inode_getsecctx(struct inode *inode, struct lsm_context *cp)
 {
 	struct smack_known *skp = smk_of_inode(inode);
 
-	*ctx = skp->smk_known;
-	*ctxlen = strlen(skp->smk_known);
+	cp->context = skp->smk_known;
+	cp->len = strlen(skp->smk_known);
+	cp->id = LSM_ID_SMACK;
 	return 0;
 }
 
-- 
2.46.0

Re: [PATCH v3 3/5] LSM: Use lsm_context in security_inode_getsecctx

[Paul Moore]

On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> Change the security_inode_getsecctx() interface to fill a lsm_context
> structure instead of data and length pointers.  This provides
> the information about which LSM created the context so that
> security_release_secctx() can use the correct hook.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-nfs@vger.kernel.org
> ---
>  fs/nfsd/nfs4xdr.c             | 26 ++++++++++----------------
>  include/linux/lsm_hook_defs.h |  4 ++--
>  include/linux/security.h      |  5 +++--
>  security/security.c           | 12 ++++++------
>  security/selinux/hooks.c      | 10 ++++++----
>  security/smack/smack_lsm.c    |  7 ++++---
>  6 files changed, 31 insertions(+), 33 deletions(-)

See my note on patch 1/5, merging into lsm/dev.

--
paul-moore.com

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

[Casey Schaufler]

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/ceph/super.h b/fs/ceph/super.h
index 2508aa8950b7..c9fad8c825dd 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -1133,8 +1133,7 @@ struct ceph_acl_sec_ctx {
 	void *acl;
 #endif
 #ifdef CONFIG_CEPH_FS_SECURITY_LABEL
-	void *sec_ctx;
-	u32 sec_ctxlen;
+	struct lsm_context lsmctx;
 #endif
 #ifdef CONFIG_FS_ENCRYPTION
 	struct ceph_fscrypt_auth *fscrypt_auth;
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index f7996770cc2c..0b9e1f385d31 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -1383,8 +1383,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 	int err;
 
 	err = security_dentry_init_security(dentry, mode, &dentry->d_name,
-					    &name, &as_ctx->sec_ctx,
-					    &as_ctx->sec_ctxlen);
+					    &name, &as_ctx->lsmctx);
 	if (err < 0) {
 		WARN_ON_ONCE(err != -EOPNOTSUPP);
 		err = 0; /* do nothing */
@@ -1409,7 +1408,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 	 */
 	name_len = strlen(name);
 	err = ceph_pagelist_reserve(pagelist,
-				    4 * 2 + name_len + as_ctx->sec_ctxlen);
+				    4 * 2 + name_len + as_ctx->lsmctx.len);
 	if (err)
 		goto out;
 
@@ -1432,8 +1431,9 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 	ceph_pagelist_encode_32(pagelist, name_len);
 	ceph_pagelist_append(pagelist, name, name_len);
 
-	ceph_pagelist_encode_32(pagelist, as_ctx->sec_ctxlen);
-	ceph_pagelist_append(pagelist, as_ctx->sec_ctx, as_ctx->sec_ctxlen);
+	ceph_pagelist_encode_32(pagelist, as_ctx->lsmctx.len);
+	ceph_pagelist_append(pagelist, as_ctx->lsmctx.context,
+			     as_ctx->lsmctx.len);
 
 	err = 0;
 out:
@@ -1446,16 +1446,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 
 void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx)
 {
-#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
-	struct lsm_context scaff; /* scaffolding */
-#endif
 #ifdef CONFIG_CEPH_FS_POSIX_ACL
 	posix_acl_release(as_ctx->acl);
 	posix_acl_release(as_ctx->default_acl);
 #endif
 #ifdef CONFIG_CEPH_FS_SECURITY_LABEL
-	lsmcontext_init(&scaff, as_ctx->sec_ctx, as_ctx->sec_ctxlen, 0);
-	security_release_secctx(&scaff);
+	security_release_secctx(&as_ctx->lsmctx);
 #endif
 #ifdef CONFIG_FS_ENCRYPTION
 	kfree(as_ctx->fscrypt_auth);
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 54104dd48af7..eea4d0d27ce1 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -466,29 +466,29 @@ static int get_security_context(struct dentry *entry, umode_t mode,
 {
 	struct fuse_secctx *fctx;
 	struct fuse_secctx_header *header;
-	void *ctx = NULL, *ptr;
-	u32 ctxlen, total_len = sizeof(*header);
+	struct lsm_context lsmctx = { };
+	void *ptr;
+	u32 total_len = sizeof(*header);
 	int err, nr_ctx = 0;
-	const char *name;
+	const char *name = NULL;
 	size_t namelen;
 
 	err = security_dentry_init_security(entry, mode, &entry->d_name,
-					    &name, &ctx, &ctxlen);
-	if (err) {
-		if (err != -EOPNOTSUPP)
-			goto out_err;
-		/* No LSM is supporting this security hook. Ignore error */
-		ctxlen = 0;
-		ctx = NULL;
-	}
+					    &name, &lsmctx);
+
+	/* If no LSM is supporting this security hook ignore error */
+	if (err && err != -EOPNOTSUPP)
+		goto out_err;
 
-	if (ctxlen) {
+	if (lsmctx.len) {
 		nr_ctx = 1;
 		namelen = strlen(name) + 1;
 		err = -EIO;
-		if (WARN_ON(namelen > XATTR_NAME_MAX + 1 || ctxlen > S32_MAX))
+		if (WARN_ON(namelen > XATTR_NAME_MAX + 1 ||
+		    lsmctx.len > S32_MAX))
 			goto out_err;
-		total_len += FUSE_REC_ALIGN(sizeof(*fctx) + namelen + ctxlen);
+		total_len += FUSE_REC_ALIGN(sizeof(*fctx) + namelen +
+					    lsmctx.len);
 	}
 
 	err = -ENOMEM;
@@ -501,19 +501,20 @@ static int get_security_context(struct dentry *entry, umode_t mode,
 	ptr += sizeof(*header);
 	if (nr_ctx) {
 		fctx = ptr;
-		fctx->size = ctxlen;
+		fctx->size = lsmctx.len;
 		ptr += sizeof(*fctx);
 
 		strcpy(ptr, name);
 		ptr += namelen;
 
-		memcpy(ptr, ctx, ctxlen);
+		memcpy(ptr, lsmctx.context, lsmctx.len);
 	}
 	ext->size = total_len;
 	ext->value = header;
 	err = 0;
 out_err:
-	kfree(ctx);
+	if (nr_ctx)
+		security_release_secctx(&lsmctx);
 	return err;
 }
 
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;
+		security_release_secctx(&shim);
 	}
 }
 static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 69e1076448c6..e2f1ce37c41e 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -83,7 +83,7 @@ LSM_HOOK(int, 0, move_mount, const struct path *from_path,
 	 const struct path *to_path)
 LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
 	 int mode, const struct qstr *name, const char **xattr_name,
-	 void **ctx, u32 *ctxlen)
+	 struct lsm_context *cp)
 LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
 	 struct qstr *name, const struct cred *old, struct cred *new)
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 7d0adc1833ab..3ad59666e56c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -237,25 +237,6 @@ struct lsm_context {
 	int	id;		/* Identifies the module */
 };
 
-/**
- * lsmcontext_init - initialize an lsmcontext structure.
- * @cp: Pointer to the context to initialize
- * @context: Initial context, or NULL
- * @size: Size of context, or 0
- * @id: Which LSM provided the context
- *
- * Fill in the lsmcontext from the provided information.
- * This is a scaffolding function that will be removed when
- * lsm_context integration is complete.
- */
-static inline void lsmcontext_init(struct lsm_context *cp, char *context,
-				   u32 size, int id)
-{
-	cp->id = id;
-	cp->context = context;
-	cp->len = size;
-}
-
 /*
  * Values used in the task_security_ops calls
  */
@@ -409,8 +390,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
 int security_move_mount(const struct path *from_path, const struct path *to_path);
 int security_dentry_init_security(struct dentry *dentry, int mode,
 				  const struct qstr *name,
-				  const char **xattr_name, void **ctx,
-				  u32 *ctxlen);
+				  const char **xattr_name,
+				  struct lsm_context *lsmcxt);
 int security_dentry_create_files_as(struct dentry *dentry, int mode,
 					struct qstr *name,
 					const struct cred *old,
@@ -883,8 +864,7 @@ static inline int security_dentry_init_security(struct dentry *dentry,
 						 int mode,
 						 const struct qstr *name,
 						 const char **xattr_name,
-						 void **ctx,
-						 u32 *ctxlen)
+						 struct lsm_context *lsmcxt)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/security.c b/security/security.c
index 4ca3c9e28b6f..1d57e4e1bceb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1734,8 +1734,7 @@ void security_inode_free(struct inode *inode)
  * @mode: mode used to determine resource type
  * @name: name of the last path component
  * @xattr_name: name of the security/LSM xattr
- * @ctx: pointer to the resulting LSM context
- * @ctxlen: length of @ctx
+ * @lsmctx: pointer to the resulting LSM context
  *
  * Compute a context for a dentry as the inode is not yet available since NFSv4
  * has no label backed by an EA anyway.  It is important to note that
@@ -1745,11 +1744,11 @@ void security_inode_free(struct inode *inode)
  */
 int security_dentry_init_security(struct dentry *dentry, int mode,
 				  const struct qstr *name,
-				  const char **xattr_name, void **ctx,
-				  u32 *ctxlen)
+				  const char **xattr_name,
+				  struct lsm_context *lsmctx)
 {
 	return call_int_hook(dentry_init_security, dentry, mode, name,
-			     xattr_name, ctx, ctxlen);
+			     xattr_name, lsmctx);
 }
 EXPORT_SYMBOL(security_dentry_init_security);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ce5e45abd8d3..79776a5e651d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2869,8 +2869,8 @@ static void selinux_inode_free_security(struct inode *inode)
 
 static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 					const struct qstr *name,
-					const char **xattr_name, void **ctx,
-					u32 *ctxlen)
+					const char **xattr_name,
+					struct lsm_context *cp)
 {
 	u32 newsid;
 	int rc;
@@ -2885,8 +2885,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 	if (xattr_name)
 		*xattr_name = XATTR_NAME_SELINUX;
 
-	return security_sid_to_context(newsid, (char **)ctx,
-				       ctxlen);
+	cp->id = LSM_ID_SELINUX;
+	return security_sid_to_context(newsid, (char **)cp->context,
+				       &cp->len);
 }
 
 static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
-- 
2.46.0

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

[Paul Moore]

On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> 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(-)

See my note on patch 1/5, merging into lsm/dev.

--
paul-moore.com

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

[Stephen Smalley]

On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> 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.

> +               security_release_secctx(&shim);
>         }
>  }
>  static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)

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

[Paul Moore]

On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > 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.

-- 
paul-moore.com

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

[Casey Schaufler]

On 2/20/2025 9:40 AM, Paul Moore wrote:
> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> 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.

I'm looking into this but as you well know the NFS tests aren't
always especially cooperative.

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

[Paul Moore]

On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >
> > > 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.

-- 
paul-moore.com

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

[Stephen Smalley]

On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >
> > > > 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.

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

[Casey Schaufler]

On 2/20/2025 10:02 AM, Stephen Smalley wrote:
> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
>>> <stephen.smalley.work@gmail.com> wrote:
>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> 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.

This is all an artifact of breaking up and rearranging the full
stacking patches. My bad. I have to find where the logic for releasing
this got lost.

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

[Stephen Smalley]

On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >
> > > > > 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>
---
 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

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

[Casey Schaufler]

On 2/20/2025 10:16 AM, Stephen Smalley wrote:
> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> 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.

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

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

[Stephen Smalley]

On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/20/2025 10:16 AM, Stephen Smalley wrote:
> > On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> >> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
> >>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> >>>> <stephen.smalley.work@gmail.com> wrote:
> >>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>> 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.

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

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

[Casey Schaufler]

On 2/20/2025 11:37 AM, Stephen Smalley wrote:
> On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/20/2025 10:16 AM, Stephen Smalley wrote:
>>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
>>> <stephen.smalley.work@gmail.com> wrote:
>>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
>>>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>> 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.

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

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

[Stephen Smalley]

On Thu, Feb 20, 2025 at 3:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/20/2025 11:37 AM, Stephen Smalley wrote:
> > On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/20/2025 10:16 AM, Stephen Smalley wrote:
> >>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
> >>> <stephen.smalley.work@gmail.com> wrote:
> >>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
> >>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
> >>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
> >>>>>> <stephen.smalley.work@gmail.com> wrote:
> >>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>>>> 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, but I don't really care as long as it
is fixed, preferably before 6.14 goes final.

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

[Casey Schaufler]

On 2/20/2025 12:33 PM, Stephen Smalley wrote:
> On Thu, Feb 20, 2025 at 3:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/20/2025 11:37 AM, Stephen Smalley wrote:
>>> On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 2/20/2025 10:16 AM, Stephen Smalley wrote:
>>>>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
>>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
>>>>>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>> 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.
>

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

[Paul Moore]

On Thu, Feb 20, 2025 at 4:08 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> 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.

Sorry for the delay today, I was distracted by some meetings and
wrestling with gcc v15 on my Fedora Rawhide dev/test system while I
try to test the LSM init patchset.  Anyway, I'll add my comments to
Stephen's formal patch posting.

-- 
paul-moore.com

[PATCH v3 5/5] LSM: secctx provider check on release

[Casey Schaufler]

Verify that the LSM releasing the secctx is the LSM that
allocated it. This was not necessary when only one LSM could
create a secctx, but once there can be more than one it is.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/apparmor/secid.c | 13 +++++--------
 security/selinux/hooks.c  | 13 +++++--------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
index 5d92fc3ab8b4..854613e58e34 100644
--- a/security/apparmor/secid.c
+++ b/security/apparmor/secid.c
@@ -122,14 +122,11 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 
 void apparmor_release_secctx(struct lsm_context *cp)
 {
-	/*
-	 * stacking scaffolding:
-	 * When it is possible for more than one LSM to provide a
-	 * release hook, do this check:
-	 * if (cp->id == LSM_ID_APPARMOR || cp->id == LSM_ID_UNDEF)
-	 */
-
-	kfree(cp->context);
+	if (cp->id == LSM_ID_APPARMOR) {
+		kfree(cp->context);
+		cp->context = NULL;
+		cp->id = LSM_ID_UNDEF;
+	}
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79776a5e651d..996e765b6823 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6640,14 +6640,11 @@ static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 
 static void selinux_release_secctx(struct lsm_context *cp)
 {
-	/*
-	 * stacking scaffolding:
-	 * When it is possible for more than one LSM to provide a
-	 * release hook, do this check:
-	 * if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF)
-	 */
-
-	kfree(cp->context);
+	if (cp->id == LSM_ID_SELINUX) {
+		kfree(cp->context);
+		cp->context = NULL;
+		cp->id = LSM_ID_UNDEF;
+	}
 }
 
 static void selinux_inode_invalidate_secctx(struct inode *inode)
-- 
2.46.0

Re: [PATCH v3 5/5] LSM: secctx provider check on release

[Paul Moore]

On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> Verify that the LSM releasing the secctx is the LSM that
> allocated it. This was not necessary when only one LSM could
> create a secctx, but once there can be more than one it is.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  security/apparmor/secid.c | 13 +++++--------
>  security/selinux/hooks.c  | 13 +++++--------
>  2 files changed, 10 insertions(+), 16 deletions(-)

See my note on patch 1/5, merging into lsm/dev.

--
paul-moore.com