* [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser [not found] <20241014151450.73674-1-casey@schaufler-ca.com> @ 2024-10-14 15:14 ` Casey Schaufler 2024-10-15 3:40 ` sergeh 2024-10-21 23:39 ` Paul Moore 2024-10-14 15:14 ` [PATCH v2 3/6] LSM: Use lsm_context in security_inode_getsecctx Casey Schaufler 2024-10-14 15:14 ` [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security Casey Schaufler 2 siblings, 2 replies; 11+ messages in thread From: Casey Schaufler @ 2024-10-14 15:14 UTC (permalink / raw) To: casey, paul, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-integrity, netdev, audit, netfilter-devel, linux-nfs, Todd Kjos 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 | 45 +++++++++++-------------- 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, 167 insertions(+), 110 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..8303bbcfc543 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)) @@ -438,11 +437,10 @@ int netlbl_unlhsh_add(struct net *net, unlhsh_add_return: 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); + if (security_secid_to_secctx(secid, &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 +471,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, @@ -494,10 +491,10 @@ 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, - &secctx, &secctx_len) == 0) { - audit_log_format(audit_buf, " sec_obj=%s", secctx); - security_release_secctx(secctx, secctx_len); + security_secid_to_secctx(entry->secid, &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 +531,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); @@ -554,10 +550,10 @@ static int netlbl_unlhsh_remove_addr6(struct net *net, addr, mask); 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); + security_secid_to_secctx(entry->secid, &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 +1065,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 +1122,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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser 2024-10-14 15:14 ` [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser Casey Schaufler @ 2024-10-15 3:40 ` sergeh 2024-10-21 23:39 ` Paul Moore 1 sibling, 0 replies; 11+ messages in thread From: sergeh @ 2024-10-15 3:40 UTC (permalink / raw) To: Casey Schaufler Cc: paul, linux-security-module, jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-integrity, netdev, audit, netfilter-devel, linux-nfs, Todd Kjos On Mon, Oct 14, 2024 at 08:14:45AM -0700, Casey Schaufler 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> Thanks. Reviewed-by: Serge Hallyn <sergeh@kernel.org> for the set (as far as affects lsm, of course). > 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 | 45 +++++++++++-------------- > 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, 167 insertions(+), 110 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..8303bbcfc543 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)) > @@ -438,11 +437,10 @@ int netlbl_unlhsh_add(struct net *net, > unlhsh_add_return: > 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); > + if (security_secid_to_secctx(secid, &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 +471,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, > @@ -494,10 +491,10 @@ 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, > - &secctx, &secctx_len) == 0) { > - audit_log_format(audit_buf, " sec_obj=%s", secctx); > - security_release_secctx(secctx, secctx_len); > + security_secid_to_secctx(entry->secid, &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 +531,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); > @@ -554,10 +550,10 @@ static int netlbl_unlhsh_remove_addr6(struct net *net, > addr, mask); > 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); > + security_secid_to_secctx(entry->secid, &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 +1065,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 +1122,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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser 2024-10-14 15:14 ` [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser Casey Schaufler 2024-10-15 3:40 ` sergeh @ 2024-10-21 23:39 ` Paul Moore 2024-10-21 23:58 ` Casey Schaufler 1 sibling, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-10-21 23:39 UTC (permalink / raw) To: Casey Schaufler, casey, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-integrity, netdev, audit, netfilter-devel, linux-nfs, Todd Kjos On Oct 14, 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> > Reviewed-by: Serge Hallyn <sergeh@kernel.org> > --- > 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 | 45 +++++++++++-------------- > 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, 167 insertions(+), 110 deletions(-) ... > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index 1bc2d0890a9f..8303bbcfc543 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -1127,14 +1122,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); Nitpicky alignment issue; please keep the arguments aligned as they are currently. > + security_release_secctx(&ctx); > if (ret_val != 0) > goto list_cb_failure; > -- paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser 2024-10-21 23:39 ` Paul Moore @ 2024-10-21 23:58 ` Casey Schaufler 2024-10-22 16:25 ` Paul Moore 0 siblings, 1 reply; 11+ messages in thread From: Casey Schaufler @ 2024-10-21 23:58 UTC (permalink / raw) To: Paul Moore, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-integrity, netdev, audit, netfilter-devel, linux-nfs, Todd Kjos, Casey Schaufler On 10/21/2024 4:39 PM, Paul Moore wrote: > On Oct 14, 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> >> Reviewed-by: Serge Hallyn <sergeh@kernel.org> >> --- >> 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 | 45 +++++++++++-------------- >> 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, 167 insertions(+), 110 deletions(-) > .. > >> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c >> index 1bc2d0890a9f..8303bbcfc543 100644 >> --- a/net/netlabel/netlabel_unlabeled.c >> +++ b/net/netlabel/netlabel_unlabeled.c >> @@ -1127,14 +1122,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); > Nitpicky alignment issue; please keep the arguments aligned as they > are currently. Not a problem, although it looks like it's correct to me. I'll check to make sure. > >> + security_release_secctx(&ctx); >> if (ret_val != 0) >> goto list_cb_failure; >> > -- > paul-moore.com > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser 2024-10-21 23:58 ` Casey Schaufler @ 2024-10-22 16:25 ` Paul Moore 0 siblings, 0 replies; 11+ messages in thread From: Paul Moore @ 2024-10-22 16:25 UTC (permalink / raw) To: Casey Schaufler Cc: linux-security-module, jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-integrity, netdev, audit, netfilter-devel, linux-nfs, Todd Kjos On Mon, Oct 21, 2024 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/21/2024 4:39 PM, Paul Moore wrote: > > On Oct 14, 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> > >> Reviewed-by: Serge Hallyn <sergeh@kernel.org> > >> --- > >> 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 | 45 +++++++++++-------------- > >> 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, 167 insertions(+), 110 deletions(-) > > .. > > > >> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > >> index 1bc2d0890a9f..8303bbcfc543 100644 > >> --- a/net/netlabel/netlabel_unlabeled.c > >> +++ b/net/netlabel/netlabel_unlabeled.c > >> @@ -1127,14 +1122,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); > > Nitpicky alignment issue; please keep the arguments aligned as they > > are currently. > > Not a problem, although it looks like it's correct to me. I'll check to make sure. Thanks. It's likely just an oddity due to tabs rendering a bit odd in the diff, I usually check that but maybe I didn't/forgot here. Not a major problem either way, I only mentioned it because I was commenting on other patches in the series. -- paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/6] LSM: Use lsm_context in security_inode_getsecctx [not found] <20241014151450.73674-1-casey@schaufler-ca.com> 2024-10-14 15:14 ` [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser Casey Schaufler @ 2024-10-14 15:14 ` Casey Schaufler 2024-10-14 15:14 ` [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security Casey Schaufler 2 siblings, 0 replies; 11+ messages in thread From: Casey Schaufler @ 2024-10-14 15:14 UTC (permalink / raw) To: casey, paul, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, linux-nfs 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security [not found] <20241014151450.73674-1-casey@schaufler-ca.com> 2024-10-14 15:14 ` [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser Casey Schaufler 2024-10-14 15:14 ` [PATCH v2 3/6] LSM: Use lsm_context in security_inode_getsecctx Casey Schaufler @ 2024-10-14 15:14 ` Casey Schaufler 2024-10-21 23:39 ` Paul Moore 2 siblings, 1 reply; 11+ messages in thread From: Casey Schaufler @ 2024-10-14 15:14 UTC (permalink / raw) To: casey, paul, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, ceph-devel, linux-nfs 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. Special care is taken in the NFS code, which uses the same data structure for its own copied labels as it does for the data which comes from security_dentry_init_security(). In the case of copied labels the data has to be freed, not released. The scaffolding funtion lsmcontext_init() is no longer needed and is removed. 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/dir.c | 2 +- fs/nfs/inode.c | 17 ++++++++++------- fs/nfs/internal.h | 8 +++++--- fs/nfs/nfs4proc.c | 22 +++++++++------------- fs/nfs/nfs4xdr.c | 22 ++++++++++++---------- include/linux/lsm_hook_defs.h | 2 +- include/linux/nfs4.h | 8 ++++---- include/linux/nfs_fs.h | 2 +- include/linux/security.h | 26 +++----------------------- security/security.c | 9 ++++----- security/selinux/hooks.c | 9 +++++---- 14 files changed, 80 insertions(+), 101 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/dir.c b/fs/nfs/dir.c index 492cffd9d3d8..1813ad9e9320 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -809,7 +809,7 @@ static int nfs_readdir_entry_decode(struct nfs_readdir_descriptor *desc, int ret; if (entry->fattr->label) - entry->fattr->label->len = NFS4_MAXLABELLEN; + entry->fattr->label->lsmctx.len = NFS4_MAXLABELLEN; ret = xdr_decode(desc, entry, stream); if (ret || !desc->plus) return ret; diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 542c7d97b235..d00a6304133a 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -358,14 +358,15 @@ void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr) return; if ((fattr->valid & NFS_ATTR_FATTR_V4_SECURITY_LABEL) && inode->i_security) { - error = security_inode_notifysecctx(inode, fattr->label->label, - fattr->label->len); + error = security_inode_notifysecctx(inode, + fattr->label->lsmctx.context, + fattr->label->lsmctx.len); if (error) printk(KERN_ERR "%s() %s %d " "security_inode_notifysecctx() %d\n", __func__, - (char *)fattr->label->label, - fattr->label->len, error); + (char *)fattr->label->lsmctx.context, + fattr->label->lsmctx.len, error); nfs_clear_label_invalid(inode); } } @@ -381,12 +382,14 @@ struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags) if (label == NULL) return ERR_PTR(-ENOMEM); - label->label = kzalloc(NFS4_MAXLABELLEN, flags); - if (label->label == NULL) { + label->lsmctx.context = kzalloc(NFS4_MAXLABELLEN, flags); + if (label->lsmctx.context == NULL) { kfree(label); return ERR_PTR(-ENOMEM); } - label->len = NFS4_MAXLABELLEN; + label->lsmctx.len = NFS4_MAXLABELLEN; + /* Use an invalid LSM ID as this should never be "released". */ + label->lsmctx.id = LSM_ID_UNDEF; return label; } diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index 430733e3eff2..96477a57f65a 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -355,13 +355,15 @@ nfs4_label_copy(struct nfs4_label *dst, struct nfs4_label *src) if (!dst || !src) return NULL; - if (src->len > NFS4_MAXLABELLEN) + if (src->lsmctx.len > NFS4_MAXLABELLEN) return NULL; dst->lfs = src->lfs; dst->pi = src->pi; - dst->len = src->len; - memcpy(dst->label, src->label, src->len); + /* Use an invalid LSM ID as lsmctx should never be "released" */ + dst->lsmctx.id = LSM_ID_UNDEF; + dst->lsmctx.len = src->lsmctx.len; + memcpy(dst->lsmctx.context, src->lsmctx.context, src->lsmctx.len); return dst; } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 76776d716744..b07d01f390dc 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -124,12 +124,11 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, label->lfs = 0; label->pi = 0; - label->len = 0; - label->label = NULL; + label->lsmctx.len = 0; + label->lsmctx.context = NULL; err = security_dentry_init_security(dentry, sattr->ia_mode, - &dentry->d_name, NULL, - (void **)&label->label, &label->len); + &dentry->d_name, NULL, &label->lsmctx); if (err == 0) return label; @@ -138,12 +137,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, static inline void nfs4_label_release_security(struct nfs4_label *label) { - struct lsm_context scaff; /* scaffolding */ - - if (label) { - lsmcontext_init(&scaff, label->label, label->len, 0); - security_release_secctx(&scaff); - } + if (label) + security_release_secctx(&label->lsmctx); } static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) { @@ -6259,7 +6254,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, {buf, buflen, -1} }; u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL }; struct nfs_fattr fattr = { @@ -6287,7 +6282,7 @@ static int _nfs4_get_security_label(struct inode *inode, void *buf, return ret; if (!(fattr.valid & NFS_ATTR_FATTR_V4_SECURITY_LABEL)) return -ENOENT; - return label.len; + return label.lsmctx.len; } static int nfs4_get_security_label(struct inode *inode, void *buf, @@ -6364,7 +6359,8 @@ 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, + {(char *)buf, buflen, -1}}; struct nfs_fattr *fattr; int status; diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index e8ac3f615f93..61a2b0e61c66 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1154,7 +1154,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, } if (label && (attrmask[2] & FATTR4_WORD2_SECURITY_LABEL)) { - len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2); + len += 4 + 4 + 4 + (XDR_QUADLEN(label->lsmctx.len) << 2); bmval[2] |= FATTR4_WORD2_SECURITY_LABEL; } @@ -1186,8 +1186,9 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, if (label && (bmval[2] & FATTR4_WORD2_SECURITY_LABEL)) { *p++ = cpu_to_be32(label->lfs); *p++ = cpu_to_be32(label->pi); - *p++ = cpu_to_be32(label->len); - p = xdr_encode_opaque_fixed(p, label->label, label->len); + *p++ = cpu_to_be32(label->lsmctx.len); + p = xdr_encode_opaque_fixed(p, label->lsmctx.context, + label->lsmctx.len); } if (bmval[2] & FATTR4_WORD2_MODE_UMASK) { *p++ = cpu_to_be32(iap->ia_mode & S_IALLUGO); @@ -4272,11 +4273,11 @@ static int decode_attr_security_label(struct xdr_stream *xdr, uint32_t *bitmap, return -EIO; bitmap[2] &= ~FATTR4_WORD2_SECURITY_LABEL; if (len < NFS4_MAXLABELLEN) { - if (label && label->len) { - if (label->len < len) + if (label && label->lsmctx.len) { + if (label->lsmctx.len < len) return -ERANGE; - memcpy(label->label, p, len); - label->len = len; + memcpy(label->lsmctx.context, p, len); + label->lsmctx.len = len; label->pi = pi; label->lfs = lfs; status = NFS_ATTR_FATTR_V4_SECURITY_LABEL; @@ -4284,10 +4285,11 @@ static int decode_attr_security_label(struct xdr_stream *xdr, uint32_t *bitmap, } else printk(KERN_WARNING "%s: label too long (%u)!\n", __func__, len); - if (label && label->label) + if (label && label->lsmctx.context) dprintk("%s: label=%.*s, len=%d, PI=%d, LFS=%d\n", - __func__, label->len, (char *)label->label, - label->len, label->pi, label->lfs); + __func__, label->lsmctx.len, + (char *)label->lsmctx.context, + label->lsmctx.len, label->pi, label->lfs); } return status; } 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/nfs4.h b/include/linux/nfs4.h index 8d7430d9f218..22032b0f6022 100644 --- a/include/linux/nfs4.h +++ b/include/linux/nfs4.h @@ -15,6 +15,7 @@ #include <linux/list.h> #include <linux/uidgid.h> +#include <linux/security.h> #include <uapi/linux/nfs4.h> #include <linux/sunrpc/msg_prot.h> @@ -44,10 +45,9 @@ struct nfs4_acl { #define NFS4_MAXLABELLEN 2048 struct nfs4_label { - uint32_t lfs; - uint32_t pi; - u32 len; - char *label; + uint32_t lfs; + uint32_t pi; + struct lsm_context lsmctx; }; typedef struct { char data[NFS4_VERIFIER_SIZE]; } nfs4_verifier; diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 039898d70954..47652d217d05 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label) { #ifdef CONFIG_NFS_V4_SECURITY_LABEL if (label) { - kfree(label->label); + kfree(label->lsmctx.context); kfree(label); } #endif 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security 2024-10-14 15:14 ` [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security Casey Schaufler @ 2024-10-21 23:39 ` Paul Moore 2024-10-22 0:00 ` Casey Schaufler 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-10-21 23:39 UTC (permalink / raw) To: Casey Schaufler, casey, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, ceph-devel, linux-nfs On Oct 14, 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. > > Special care is taken in the NFS code, which uses the same data structure > for its own copied labels as it does for the data which comes from > security_dentry_init_security(). In the case of copied labels the data > has to be freed, not released. > > The scaffolding funtion lsmcontext_init() is no longer needed and is > removed. > > 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/dir.c | 2 +- > fs/nfs/inode.c | 17 ++++++++++------- > fs/nfs/internal.h | 8 +++++--- > fs/nfs/nfs4proc.c | 22 +++++++++------------- > fs/nfs/nfs4xdr.c | 22 ++++++++++++---------- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/nfs4.h | 8 ++++---- > include/linux/nfs_fs.h | 2 +- > include/linux/security.h | 26 +++----------------------- > security/security.c | 9 ++++----- > security/selinux/hooks.c | 9 +++++---- > 14 files changed, 80 insertions(+), 101 deletions(-) ... > diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h > index 039898d70954..47652d217d05 100644 > --- a/include/linux/nfs_fs.h > +++ b/include/linux/nfs_fs.h > @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label) > { > #ifdef CONFIG_NFS_V4_SECURITY_LABEL > if (label) { > - kfree(label->label); > + kfree(label->lsmctx.context); Shouldn't this be a call to security_release_secctx() instead of a raw kfree()? > kfree(label); > } > #endif -- paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security 2024-10-21 23:39 ` Paul Moore @ 2024-10-22 0:00 ` Casey Schaufler 2024-10-22 16:35 ` Paul Moore 0 siblings, 1 reply; 11+ messages in thread From: Casey Schaufler @ 2024-10-22 0:00 UTC (permalink / raw) To: Paul Moore, linux-security-module Cc: jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, ceph-devel, linux-nfs, Casey Schaufler On 10/21/2024 4:39 PM, Paul Moore wrote: > On Oct 14, 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. >> >> Special care is taken in the NFS code, which uses the same data structure >> for its own copied labels as it does for the data which comes from >> security_dentry_init_security(). In the case of copied labels the data >> has to be freed, not released. >> >> The scaffolding funtion lsmcontext_init() is no longer needed and is >> removed. >> >> 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/dir.c | 2 +- >> fs/nfs/inode.c | 17 ++++++++++------- >> fs/nfs/internal.h | 8 +++++--- >> fs/nfs/nfs4proc.c | 22 +++++++++------------- >> fs/nfs/nfs4xdr.c | 22 ++++++++++++---------- >> include/linux/lsm_hook_defs.h | 2 +- >> include/linux/nfs4.h | 8 ++++---- >> include/linux/nfs_fs.h | 2 +- >> include/linux/security.h | 26 +++----------------------- >> security/security.c | 9 ++++----- >> security/selinux/hooks.c | 9 +++++---- >> 14 files changed, 80 insertions(+), 101 deletions(-) > .. > >> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h >> index 039898d70954..47652d217d05 100644 >> --- a/include/linux/nfs_fs.h >> +++ b/include/linux/nfs_fs.h >> @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label) >> { >> #ifdef CONFIG_NFS_V4_SECURITY_LABEL >> if (label) { >> - kfree(label->label); >> + kfree(label->lsmctx.context); > Shouldn't this be a call to security_release_secctx() instead of a raw > kfree()? As mentioned in the description, the NFS data is a copy that NFS manages, so it does need to be freed, not released. > >> kfree(label); >> } >> #endif > -- > paul-moore.com > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security 2024-10-22 0:00 ` Casey Schaufler @ 2024-10-22 16:35 ` Paul Moore 2024-10-22 16:46 ` Casey Schaufler 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-10-22 16:35 UTC (permalink / raw) To: Casey Schaufler Cc: linux-security-module, jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, ceph-devel, linux-nfs On Mon, Oct 21, 2024 at 8:00 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/21/2024 4:39 PM, Paul Moore wrote: > > On Oct 14, 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. > >> > >> Special care is taken in the NFS code, which uses the same data structure > >> for its own copied labels as it does for the data which comes from > >> security_dentry_init_security(). In the case of copied labels the data > >> has to be freed, not released. > >> > >> The scaffolding funtion lsmcontext_init() is no longer needed and is > >> removed. > >> > >> 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/dir.c | 2 +- > >> fs/nfs/inode.c | 17 ++++++++++------- > >> fs/nfs/internal.h | 8 +++++--- > >> fs/nfs/nfs4proc.c | 22 +++++++++------------- > >> fs/nfs/nfs4xdr.c | 22 ++++++++++++---------- > >> include/linux/lsm_hook_defs.h | 2 +- > >> include/linux/nfs4.h | 8 ++++---- > >> include/linux/nfs_fs.h | 2 +- > >> include/linux/security.h | 26 +++----------------------- > >> security/security.c | 9 ++++----- > >> security/selinux/hooks.c | 9 +++++---- > >> 14 files changed, 80 insertions(+), 101 deletions(-) ... > >> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h > >> index 039898d70954..47652d217d05 100644 > >> --- a/include/linux/nfs_fs.h > >> +++ b/include/linux/nfs_fs.h > >> @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label) > >> { > >> #ifdef CONFIG_NFS_V4_SECURITY_LABEL > >> if (label) { > >> - kfree(label->label); > >> + kfree(label->lsmctx.context); > > Shouldn't this be a call to security_release_secctx() instead of a raw > > kfree()? > > As mentioned in the description, the NFS data is a copy that NFS > manages, so it does need to be freed, not released. It does, my apologies. However, this makes me wonder if using the lsm_context struct for the private NFS copy is the right decision. The NFS code assumes and requires a single string, ala secctx, but I think we want the ability to potentially do other/additional things with lsm_context, even if this patchset doesn't do that. I would suggest keeping the NFS private copy as sec_ctx/sec_ctxlen and keep the concept of a translation between the data structures in place, even though it is just a simple string duplication right now. -- paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security 2024-10-22 16:35 ` Paul Moore @ 2024-10-22 16:46 ` Casey Schaufler 0 siblings, 0 replies; 11+ messages in thread From: Casey Schaufler @ 2024-10-22 16:46 UTC (permalink / raw) To: Paul Moore Cc: linux-security-module, jmorris, serge, keescook, john.johansen, penguin-kernel, stephen.smalley.work, linux-kernel, selinux, mic, ceph-devel, linux-nfs, Casey Schaufler On 10/22/2024 9:35 AM, Paul Moore wrote: > On Mon, Oct 21, 2024 at 8:00 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 10/21/2024 4:39 PM, Paul Moore wrote: >>> On Oct 14, 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. >>>> >>>> Special care is taken in the NFS code, which uses the same data structure >>>> for its own copied labels as it does for the data which comes from >>>> security_dentry_init_security(). In the case of copied labels the data >>>> has to be freed, not released. >>>> >>>> The scaffolding funtion lsmcontext_init() is no longer needed and is >>>> removed. >>>> >>>> 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/dir.c | 2 +- >>>> fs/nfs/inode.c | 17 ++++++++++------- >>>> fs/nfs/internal.h | 8 +++++--- >>>> fs/nfs/nfs4proc.c | 22 +++++++++------------- >>>> fs/nfs/nfs4xdr.c | 22 ++++++++++++---------- >>>> include/linux/lsm_hook_defs.h | 2 +- >>>> include/linux/nfs4.h | 8 ++++---- >>>> include/linux/nfs_fs.h | 2 +- >>>> include/linux/security.h | 26 +++----------------------- >>>> security/security.c | 9 ++++----- >>>> security/selinux/hooks.c | 9 +++++---- >>>> 14 files changed, 80 insertions(+), 101 deletions(-) > .. > >>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h >>>> index 039898d70954..47652d217d05 100644 >>>> --- a/include/linux/nfs_fs.h >>>> +++ b/include/linux/nfs_fs.h >>>> @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label) >>>> { >>>> #ifdef CONFIG_NFS_V4_SECURITY_LABEL >>>> if (label) { >>>> - kfree(label->label); >>>> + kfree(label->lsmctx.context); >>> Shouldn't this be a call to security_release_secctx() instead of a raw >>> kfree()? >> As mentioned in the description, the NFS data is a copy that NFS >> manages, so it does need to be freed, not released. > It does, my apologies. > > However, this makes me wonder if using the lsm_context struct for the > private NFS copy is the right decision. The NFS code assumes and > requires a single string, ala secctx, but I think we want the ability > to potentially do other/additional things with lsm_context, even if > this patchset doesn't do that. This came down to a choice about where the ugly code would be. I'll restore the old behavior. > > I would suggest keeping the NFS private copy as sec_ctx/sec_ctxlen and > keep the concept of a translation between the data structures in > place, even though it is just a simple string duplication right now. > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-10-22 16:47 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241014151450.73674-1-casey@schaufler-ca.com>
2024-10-14 15:14 ` [PATCH v2 1/6] LSM: Ensure the correct LSM context releaser Casey Schaufler
2024-10-15 3:40 ` sergeh
2024-10-21 23:39 ` Paul Moore
2024-10-21 23:58 ` Casey Schaufler
2024-10-22 16:25 ` Paul Moore
2024-10-14 15:14 ` [PATCH v2 3/6] LSM: Use lsm_context in security_inode_getsecctx Casey Schaufler
2024-10-14 15:14 ` [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security Casey Schaufler
2024-10-21 23:39 ` Paul Moore
2024-10-22 0:00 ` Casey Schaufler
2024-10-22 16:35 ` Paul Moore
2024-10-22 16:46 ` Casey Schaufler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox