On 12/16/19 5:36 PM, Casey Schaufler wrote:
Add a new lsmcontext 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.
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
Reviewed-by: John Johansen <john.johansen@xxxxxxxxxxxxx>
Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
cc: linux-integrity@xxxxxxxxxxxxxxx
cc: netdev@xxxxxxxxxxxxxxx
---
drivers/android/binder.c | 10 +++++--
fs/ceph/xattr.c | 6 +++-
fs/nfs/nfs4proc.c | 8 +++--
fs/nfsd/nfs4xdr.c | 7 +++--
include/linux/security.h | 39 +++++++++++++++++++++++--
include/net/scm.h | 5 +++-
kernel/audit.c | 14 ++++++---
kernel/auditsc.c | 12 ++++++--
net/ipv4/ip_sockglue.c | 4 ++-
net/netfilter/nf_conntrack_netlink.c | 4 ++-
net/netfilter/nf_conntrack_standalone.c | 4 ++-
net/netfilter/nfnetlink_queue.c | 13 ++++++---
net/netlabel/netlabel_unlabeled.c | 19 +++++++++---
net/netlabel/netlabel_user.c | 4 ++-
security/security.c | 18 ++++++++----
security/smack/smack_lsm.c | 14 ++++++---
16 files changed, 141 insertions(+), 40 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index d12b5e828b8d..597d9802b89b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -128,6 +128,41 @@ enum lockdown_reason {
LOCKDOWN_CONFIDENTIALITY_MAX,
};
+/*
+ * 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 lsmcontext {
+ char *context; /* Provided by the module */
+ u32 len;
+ int slot; /* 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
+ * @slot: Which LSM provided the context
+ *
+ * Fill in the lsmcontext from the provided information.
+ * This is a scaffolding function that will be removed when
+ * lsmcontext integration is complete.
+ */
+static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
+ u32 size, int slot)
+{
+ cp->slot = slot;
+ cp->context = context;
+
+ if (context == NULL || size == 0)
+ cp->len = 0;
+ else
+ cp->len = strlen(context);
+}
Why do you recompute the length instead of just storing size? Aside from
being less efficient, it may also be incorrect; SELinux-generated
contexts include the terminating NUL byte.
diff --git a/security/security.c b/security/security.c
index aaac748e4d83..6310ca7e84ed 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2144,17 +2144,23 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
}
EXPORT_SYMBOL(security_secctx_to_secid);
-void security_release_secctx(char *secdata, u32 seclen)
+void security_release_secctx(struct lsmcontext *cp)
{
struct security_hook_list *hp;
- int *display = current->security;
+ bool found = false;
hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
- if (display == NULL || *display == LSMBLOB_INVALID ||
- *display == hp->lsmid->slot) {
- hp->hook.release_secctx(secdata, seclen);
- return;
+ if (cp->slot == hp->lsmid->slot) {
+ hp->hook.release_secctx(cp->context, cp->len);
+ found = true;
+ break;
}
+
+ memset(cp, 0, sizeof(*cp));
+
+ if (!found)
+ pr_warn("%s context \"%s\" from slot %d not released\n",
+ __func__, cp->context, cp->slot);
Not sure we need this warning but regardless, you cleared cp before the
pr_warn() so the output won't be very useful.
}
EXPORT_SYMBOL(security_release_secctx);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9737ead06b39..8e960f82bf3f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4482,11 +4482,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
return 0;
}
-/*
- * There used to be a smack_release_secctx hook
- * that did nothing back when hooks were in a vector.
- * Now that there's a list such a hook adds cost.
+/**
+ * smack_release_secctx - do everything necessary to free a context
+ * @secdata: Unused
+ * @seclen: Unused
+ *
+ * Do nothing but hold a slot in the hooks list.
*/
+static void smack_release_secctx(char *secdata, u32 seclen)
+{
+}
static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
{
@@ -4729,6 +4734,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
+ LSM_HOOK_INIT(release_secctx, smack_release_secctx),
LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
Is this just to avoid the warning above? If so, I'd just get rid of the
warning instead.
