* Re: [PATCH v2 04/25] LSM: Create and manage the lsmblob data structure.
From: Kees Cook @ 2019-06-19 4:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-5-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:30PM -0700, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 1 +
> include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++
> security/security.c | 31 ++++++++++++++++++++
> 3 files changed, 94 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 3fe39abccc8f..4d1ddf1a2aa6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2038,6 +2038,7 @@ struct security_hook_list {
> struct hlist_head *head;
> union security_list_options hook;
> char *lsm;
> + int slot;
> } __randomize_layout;
Hm, this feels redundant (as does the existing "char *lsm") now that we
have lsm_info. The place for assigned-at-init value is blob_sizes, which
hangs off of lsm_info (as does the LSM char *)...
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 49f2685324b0..28d074866895 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -76,6 +76,68 @@ enum lsm_event {
> LSM_POLICY_CHANGE,
> };
>
> +/*
> + * Data exported by the security modules
> + */
> +#define LSMDATA_ENTRIES ( \
LSMBLOB_ENTRIES?
> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) )
> +
> +struct lsmblob {
> + u32 secid[LSMDATA_ENTRIES];
> +};
Cool; I like this auto-sizing.
> +
> +#define LSMDATA_INVALID -1
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.
> + * @l: Pointer to the data to initialize
> + * @secid: The initial secid value
> + *
> + * Set all secid for all modules to the specified value.
> + */
> +static inline void lsmblob_init(struct lsmblob *l, u32 secid)
> +{
> + int i;
> +
> + for (i = 0; i < LSMDATA_ENTRIES; i++)
> + l->secid[i] = secid;
For all these LSMDATA_ENTRIES, I prefer ARRAY_SIZE(l->secid), but
*shrug*
> +}
> +
> +/**
> + * lsmblob_is_set - report if there is an value in the lsmblob
> + * @l: Pointer to the exported LSM data
> + *
> + * Returns true if there is a secid set, false otherwise
> + */
> +static inline bool lsmblob_is_set(struct lsmblob *l)
> +{
> + int i;
> +
> + for (i = 0; i < LSMDATA_ENTRIES; i++)
> + if (l->secid[i] != 0)
> + return true;
> + return false;
> +}
> +
> +/**
> + * lsmblob_equal - report if the two lsmblob's are equal
> + * @l: Pointer to one LSM data
> + * @m: Pointer to the other LSM data
> + *
> + * Returns true if all entries in the two are equal, false otherwise
> + */
> +static inline bool lsmblob_equal(struct lsmblob *l, struct lsmblob *m)
> +{
> + int i;
> +
> + for (i = 0; i < LSMDATA_ENTRIES; i++)
> + if (l->secid[i] != m->secid[i])
> + return false;
> + return true;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> diff --git a/security/security.c b/security/security.c
> index d05f00a40e82..5aa3c052d702 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void)
> init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> init_debug("task blob size = %d\n", blob_sizes.lbs_task);
> + init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob));
>
> /*
> * Create any kmem_caches needed for blobs
> @@ -420,6 +421,11 @@ static int lsm_append(char *new, char **result)
> return 0;
> }
>
> +/*
> + * Current index to use while initializing the lsmblob secid list.
> + */
> +static int lsm_slot __initdata;
> +
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> * @hooks: the hooks to add
> @@ -427,15 +433,40 @@ static int lsm_append(char *new, char **result)
> * @lsm: the name of the security module
> *
> * Each LSM has to register its hooks with the infrastructure.
> + * If the LSM is using hooks that export secids allocate a slot
> + * for it in the lsmblob.
> */
> void __init security_add_hooks(struct security_hook_list *hooks, int count,
> char *lsm)
> {
> + int slot = LSMDATA_INVALID;
> int i;
>
> for (i = 0; i < count; i++) {
> hooks[i].lsm = lsm;
> hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
> + /*
> + * If this is one of the hooks that uses a secid
> + * note it so that a slot can in allocated for the
> + * secid in the lsmblob structure.
> + */
> + if (hooks[i].head == &security_hook_heads.audit_rule_match ||
> + hooks[i].head == &security_hook_heads.kernel_act_as ||
> + hooks[i].head ==
> + &security_hook_heads.socket_getpeersec_dgram ||
> + hooks[i].head == &security_hook_heads.secctx_to_secid ||
> + hooks[i].head == &security_hook_heads.secid_to_secctx ||
> + hooks[i].head == &security_hook_heads.ipc_getsecid ||
> + hooks[i].head == &security_hook_heads.task_getsecid ||
> + hooks[i].head == &security_hook_heads.inode_getsecid ||
> + hooks[i].head == &security_hook_heads.cred_getsecid) {
> + if (slot == LSMDATA_INVALID) {
> + slot = lsm_slot++;
This needs to sanity check lsm_slot against lsmblob secids array size,
just we we catch cases cleanly if an LSM adds a hook but doesn't add
itself to the LSMDATA_ENTRIES macro.
> + init_debug("%s assigned lsmblob slot %d\n",
> + hooks[i].lsm, slot);
> + }
> + }
> + hooks[i].slot = slot;
> }
> if (lsm_append(lsm, &lsm_names) < 0)
> panic("%s - Cannot get early memory.\n", __func__);
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 05/25] Use lsmblob in security_audit_rule_match
From: Kees Cook @ 2019-06-19 4:55 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-6-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:31PM -0700, Casey Schaufler wrote:
> Change the secid parameter of security_audit_rule_match
> to a lsmblob structure pointer. Pass the entry from the
> lsmblob structure for the approprite slot to the LSM hook.
>
> Change the users of security_audit_rule_match to use the
> lsmblob instead of a u32. In some cases this requires a
> temporary conversion using lsmblob_init() that will go
> away when other interfaces get converted.
I like this much better with the LSM-infrastructure "slot" logic.
I do think it's be easier to read if the lsmblob instances were called
"blob" instead of "le" and "l"...
-Kees
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 7 ++++---
> kernel/auditfilter.c | 4 +++-
> kernel/auditsc.c | 13 +++++++++----
> security/integrity/ima/ima.h | 4 ++--
> security/integrity/ima/ima_policy.c | 7 +++++--
> security/security.c | 14 ++++++++++++--
> 6 files changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 28d074866895..067fabc63e51 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1757,7 +1757,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
> #ifdef CONFIG_SECURITY
> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> int security_audit_rule_known(struct audit_krule *krule);
> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> +int security_audit_rule_match(struct lsmblob *l, u32 field, u32 op,
> + void *lsmrule);
> void security_audit_rule_free(void *lsmrule);
>
> #else
> @@ -1773,8 +1774,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
> return 0;
> }
>
> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
> - void *lsmrule)
> +static inline int security_audit_rule_match(struct lsmblob *l, u32 field,
> + u32 op, void *lsmrule)
> {
> return 0;
> }
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 63f8b3f26fab..934ceae1ff70 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1324,6 +1324,7 @@ int audit_filter(int msgtype, unsigned int listtype)
> struct audit_field *f = &e->rule.fields[i];
> pid_t pid;
> u32 sid;
> + struct lsmblob le;
>
> switch (f->type) {
> case AUDIT_PID:
> @@ -1354,7 +1355,8 @@ int audit_filter(int msgtype, unsigned int listtype)
> case AUDIT_SUBJ_CLR:
> if (f->lsm_rule) {
> security_task_getsecid(current, &sid);
> - result = security_audit_rule_match(sid,
> + lsmblob_init(&le, sid);
> + result = security_audit_rule_match(&le,
> f->type, f->op, f->lsm_rule);
> }
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1eab1d4a930..93c74205ef40 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> const struct cred *cred;
> int i, need_sid = 1;
> u32 sid;
> + struct lsmblob le;
> unsigned int sessionid;
>
> cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
> @@ -630,7 +631,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> security_task_getsecid(tsk, &sid);
> need_sid = 0;
> }
> - result = security_audit_rule_match(sid, f->type,
> + lsmblob_init(&le, sid);
> + result = security_audit_rule_match(&le, f->type,
> f->op,
> f->lsm_rule);
> }
> @@ -645,15 +647,17 @@ static int audit_filter_rules(struct task_struct *tsk,
> if (f->lsm_rule) {
> /* Find files that match */
> if (name) {
> + lsmblob_init(&le, name->osid);
> result = security_audit_rule_match(
> - name->osid,
> + &le,
> f->type,
> f->op,
> f->lsm_rule);
> } else if (ctx) {
> list_for_each_entry(n, &ctx->names_list, list) {
> + lsmblob_init(&le, n->osid);
> if (security_audit_rule_match(
> - n->osid,
> + &le,
> f->type,
> f->op,
> f->lsm_rule)) {
> @@ -665,7 +669,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> /* Find ipc objects that match */
> if (!ctx || ctx->type != AUDIT_IPC)
> break;
> - if (security_audit_rule_match(ctx->ipc.osid,
> + lsmblob_init(&le, ctx->ipc.osid);
> + if (security_audit_rule_match(&le,
> f->type, f->op,
> f->lsm_rule))
> ++result;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..fe5e921d621d 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -307,8 +307,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> return -EINVAL;
> }
>
> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> - void *lsmrule)
> +static inline int security_filter_rule_match(struct lsmblob *l, u32 field,
> + u32 op, void *lsmrule)
> {
> return -EINVAL;
> }
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..ae525a89e07f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -327,6 +327,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> u32 osid;
> + struct lsmblob le;
> int retried = 0;
>
> if (!rule->lsm[i].rule)
> @@ -337,7 +338,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_OBJ_ROLE:
> case LSM_OBJ_TYPE:
> security_inode_getsecid(inode, &osid);
> - rc = security_filter_rule_match(osid,
> + lsmblob_init(&le, osid);
> + rc = security_filter_rule_match(&le,
> rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
> @@ -345,7 +347,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> - rc = security_filter_rule_match(secid,
> + lsmblob_init(&le, secid);
> + rc = security_filter_rule_match(&le,
> rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
> diff --git a/security/security.c b/security/security.c
> index 5aa3c052d702..45541053df89 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2447,9 +2447,19 @@ void security_audit_rule_free(void *lsmrule)
> call_void_hook(audit_rule_free, lsmrule);
> }
>
> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +int security_audit_rule_match(struct lsmblob *l, u32 field, u32 op,
> + void *lsmrule)
> {
> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> + struct security_hook_list *hp;
> + int rc;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> + rc = hp->hook.audit_rule_match(l->secid[hp->slot], field,
> + op, lsmrule);
> + if (rc != 0)
> + return rc;
> + }
> + return 0;
> }
> #endif /* CONFIG_AUDIT */
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 07/25] net: Prepare UDS for secuirty module stacking
From: Kees Cook @ 2019-06-19 4:59 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-8-casey@schaufler-ca.com>
typo in Subject -> "secuirty" -> "security"
On Tue, Jun 18, 2019 at 04:05:33PM -0700, Casey Schaufler wrote:
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
Can you spell this out a little more, "scaffolding code passes slot 1
unconditionally while the following patches will fix this up when they
are made aware of lsmblob" etc. (Also, why slot 1 and not slot 0?)
-Kees
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 7 +++++--
> include/net/af_unix.h | 2 +-
> include/net/scm.h | 8 +++++---
> net/ipv4/ip_sockglue.c | 8 +++++---
> net/unix/af_unix.c | 6 +++---
> security/security.c | 16 +++++++++++++---
> 6 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 89a5391f2441..64f5cc2dd249 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1276,7 +1276,8 @@ int security_socket_shutdown(struct socket *sock, int how);
> int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len);
> -int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
> +int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> + struct lsmblob *l);
> int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> void security_sk_free(struct sock *sk);
> void security_sk_clone(const struct sock *sk, struct sock *newsk);
> @@ -1414,7 +1415,9 @@ static inline int security_socket_getpeersec_stream(struct socket *sock, char __
> return -ENOPROTOOPT;
> }
>
> -static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> +static inline int security_socket_getpeersec_dgram(struct socket *sock,
> + struct sk_buff *skb,
> + struct lsmblob *l)
> {
> return -ENOPROTOOPT;
> }
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 3426d6dacc45..933492c08b8c 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> kgid_t gid;
> struct scm_fp_list *fp; /* Passed files */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Security ID */
> + struct lsmblob lsmblob; /* Security LSM data */
> #endif
> u32 consumed;
> } __randomize_layout;
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 1ce365f4c256..c87a17101c86 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -33,7 +33,7 @@ struct scm_cookie {
> struct scm_fp_list *fp; /* Passed files */
> struct scm_creds creds; /* Skb credentials */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Passed security ID */
> + struct lsmblob lsmblob; /* Passed LSM data */
> #endif
> };
>
> @@ -46,7 +46,7 @@ struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
> #ifdef CONFIG_SECURITY_NETWORK
> static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
> {
> - security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
> + security_socket_getpeersec_dgram(sock, NULL, &scm->lsmblob);
> }
> #else
> static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
> @@ -97,7 +97,9 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
> + /* Scaffolding - it has to be element 1 for now */
> + err = security_secid_to_secctx(scm->lsmblob.secid[1],
> + &secdata, &seclen);
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 82f341e84fae..fbe2147ee595 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,15 +130,17 @@ 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)
> {
> + struct lsmblob lb;
> char *secdata;
> - u32 seclen, secid;
> + u32 seclen;
> int err;
>
> - err = security_socket_getpeersec_dgram(NULL, skb, &secid);
> + err = security_socket_getpeersec_dgram(NULL, skb, &lb);
> if (err)
> return;
>
> - err = security_secid_to_secctx(secid, &secdata, &seclen);
> + /* Scaffolding - it has to be element 1 */
> + err = security_secid_to_secctx(lb.secid[1], &secdata, &seclen);
> if (err)
> return;
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index ddb838a1b74c..c50a004a1389 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -143,17 +143,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
> #ifdef CONFIG_SECURITY_NETWORK
> static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> {
> - UNIXCB(skb).secid = scm->secid;
> + UNIXCB(skb).lsmblob = scm->lsmblob;
> }
>
> static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> {
> - scm->secid = UNIXCB(skb).secid;
> + scm->lsmblob = UNIXCB(skb).lsmblob;
> }
>
> static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
> {
> - return (scm->secid == UNIXCB(skb).secid);
> + return lsmblob_equal(&scm->lsmblob, &(UNIXCB(skb).lsmblob));
> }
> #else
> static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> diff --git a/security/security.c b/security/security.c
> index 4296cd2ca508..5ed818699e15 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2132,10 +2132,20 @@ int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> optval, optlen, len);
> }
>
> -int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> +int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> + struct lsmblob *l)
> {
> - return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
> - skb, secid);
> + struct security_hook_list *hp;
> + int rc = -ENOPROTOOPT;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_dgram,
> + list) {
> + rc = hp->hook.socket_getpeersec_dgram(sock, skb,
> + &l->secid[hp->slot]);
> + if (rc != 0)
> + break;
> + }
> + return rc;
> }
> EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 09/25] LSM: Use lsmblob in security_secid_to_secctx
From: Kees Cook @ 2019-06-19 5:03 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-10-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:35PM -0700, Casey Schaufler wrote:
> Change security_secid_to_secctx() to take a lsmblob as input
> instead of a u32 secid. It will then call the LSM hooks
> using the lsmblob element allocated for that module. The
> callers have been updated as well. This allows for the
> possibility that more than one module may called upon
> to translate a secid to a string, as can occur in the
> audit code.
Cool. The progression of scaffolding here is pretty clear to me.
-Kees
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> drivers/android/binder.c | 4 +++-
> include/linux/security.h | 5 +++--
> include/net/scm.h | 4 +---
> kernel/audit.c | 9 +++++++--
> kernel/auditsc.c | 13 +++++++++----
> net/ipv4/ip_sockglue.c | 3 +--
> net/netfilter/nf_conntrack_netlink.c | 8 ++++++--
> net/netfilter/nf_conntrack_standalone.c | 4 +++-
> net/netfilter/nfnetlink_queue.c | 8 ++++++--
> net/netlabel/netlabel_unlabeled.c | 18 ++++++++++++++----
> net/netlabel/netlabel_user.c | 6 +++---
> security/security.c | 14 +++++++++++---
> 12 files changed, 67 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8685882da64c..a3204fbc1f28 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3120,9 +3120,11 @@ static void binder_transaction(struct binder_proc *proc,
>
> if (target_node && target_node->txn_security_ctx) {
> u32 secid;
> + struct lsmblob le;
>
> security_task_getsecid(proc->tsk, &secid);
> - ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> + lsmblob_init(&le, secid);
> + ret = security_secid_to_secctx(&le, &secctx, &secctx_sz);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dcaaa63b79b3..c9ed83e57a97 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -442,7 +442,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> +int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *l);
> void security_release_secctx(char *secdata, u32 seclen);
> @@ -1220,7 +1220,8 @@ static inline int security_ismaclabel(const char *name)
> return 0;
> }
>
> -static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> +static inline int security_secid_to_secctx(struct lsmblob *l,
> + char **secdata, u32 *seclen)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index c87a17101c86..bcb0f8560cdf 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,9 +97,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - /* Scaffolding - it has to be element 1 for now */
> - err = security_secid_to_secctx(scm->lsmblob.secid[1],
> - &secdata, &seclen);
> + err = security_secid_to_secctx(&scm->lsmblob, &secdata, &seclen);
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index c89ea48c70a6..5efd78ced915 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1430,7 +1430,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (audit_sig_sid) {
> - err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
> + struct lsmblob le;
> +
> + lsmblob_init(&le, audit_sig_sid);
> + err = security_secid_to_secctx(&le, &ctx, &len);
> if (err)
> return err;
> }
> @@ -2073,12 +2076,14 @@ int audit_log_task_context(struct audit_buffer *ab)
> unsigned len;
> int error;
> u32 sid;
> + struct lsmblob le;
>
> security_task_getsecid(current, &sid);
> if (!sid)
> return 0;
>
> - error = security_secid_to_secctx(sid, &ctx, &len);
> + lsmblob_init(&le, sid);
> + error = security_secid_to_secctx(&le, &ctx, &len);
> if (error) {
> if (error != -EINVAL)
> goto error_path;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 93c74205ef40..67d3f71a095a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -946,6 +946,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> + struct lsmblob le;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> if (!ab)
> @@ -955,7 +956,8 @@ 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 (sid) {
> - if (security_secid_to_secctx(sid, &ctx, &len)) {
> + lsmblob_init(&le, sid);
> + if (security_secid_to_secctx(&le, &ctx, &len)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> @@ -1197,7 +1199,9 @@ static void show_special(struct audit_context *context, int *call_panic)
> if (osid) {
> char *ctx = NULL;
> u32 len;
> - if (security_secid_to_secctx(osid, &ctx, &len)) {
> + struct lsmblob le;
> + lsmblob_init(&le, osid);
> + if (security_secid_to_secctx(&le, &ctx, &len)) {
> audit_log_format(ab, " osid=%u", osid);
> *call_panic = 1;
> } else {
> @@ -1348,9 +1352,10 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> if (n->osid != 0) {
> char *ctx = NULL;
> u32 len;
> + struct lsmblob le;
>
> - if (security_secid_to_secctx(
> - n->osid, &ctx, &len)) {
> + lsmblob_init(&le, n->osid);
> + if (security_secid_to_secctx(&le, &ctx, &len)) {
> audit_log_format(ab, " osid=%u", n->osid);
> if (call_panic)
> *call_panic = 2;
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index fbe2147ee595..e05f4ef68bd8 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -139,8 +139,7 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> if (err)
> return;
>
> - /* Scaffolding - it has to be element 1 */
> - err = security_secid_to_secctx(lb.secid[1], &secdata, &seclen);
> + err = security_secid_to_secctx(&lb, &secdata, &seclen);
> if (err)
> return;
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 66c596d287a5..6098b586da07 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -330,8 +330,10 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> struct nlattr *nest_secctx;
> int len, ret;
> char *secctx;
> + struct lsmblob le;
>
> - ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> + lsmblob_init(&le, ct->secmark);
> + ret = security_secid_to_secctx(&le, &secctx, &len);
> if (ret)
> return 0;
>
> @@ -615,8 +617,10 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> {
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
> int len, ret;
> + struct lsmblob le;
>
> - ret = security_secid_to_secctx(ct->secmark, NULL, &len);
> + lsmblob_init(&le, ct->secmark);
> + ret = security_secid_to_secctx(&le, NULL, &len);
> if (ret)
> return 0;
>
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index c2ae14c720b4..6e6fb1f9f6ba 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -175,8 +175,10 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> int ret;
> u32 len;
> char *secctx;
> + struct lsmblob le;
>
> - ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> + lsmblob_init(&le, ct->secmark);
> + ret = security_secid_to_secctx(&le, &secctx, &len);
> if (ret)
> return;
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 0dcc3592d053..105018d19318 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -309,13 +309,17 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> {
> u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> + struct lsmblob le;
> +
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
>
> read_lock_bh(&skb->sk->sk_callback_lock);
>
> - if (skb->secmark)
> - security_secid_to_secctx(skb->secmark, secdata, &seclen);
> + if (skb->secmark) {
> + lsmblob_init(&le, skb->secmark);
> + security_secid_to_secctx(&le, secdata, &seclen);
> + }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> #endif
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 30873e671d05..46ac9721e261 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -389,6 +389,7 @@ int netlbl_unlhsh_add(struct net *net,
> struct audit_buffer *audit_buf = NULL;
> char *secctx = NULL;
> u32 secctx_len;
> + struct lsmblob le;
>
> if (addr_len != sizeof(struct in_addr) &&
> addr_len != sizeof(struct in6_addr))
> @@ -451,7 +452,8 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - if (security_secid_to_secctx(secid,
> + lsmblob_init(&le, secid);
> + if (security_secid_to_secctx(&le,
> &secctx,
> &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> @@ -488,6 +490,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct net_device *dev;
> char *secctx;
> u32 secctx_len;
> + struct lsmblob le;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
> @@ -507,8 +510,10 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> addr->s_addr, mask->s_addr);
> if (dev != NULL)
> dev_put(dev);
> + if (entry != NULL)
> + lsmblob_init(&le, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(entry->secid,
> + security_secid_to_secctx(&le,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> security_release_secctx(secctx, secctx_len);
> @@ -550,6 +555,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct net_device *dev;
> char *secctx;
> u32 secctx_len;
> + struct lsmblob le;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
> @@ -568,8 +574,10 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> addr, mask);
> if (dev != NULL)
> dev_put(dev);
> + if (entry != NULL)
> + lsmblob_init(&le, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(entry->secid,
> + security_secid_to_secctx(&le,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> security_release_secctx(secctx, secctx_len);
> @@ -1090,6 +1098,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> u32 secid;
> char *secctx;
> u32 secctx_len;
> + struct lsmblob le;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> cb_arg->seq, &netlbl_unlabel_gnl_family,
> @@ -1144,7 +1153,8 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> secid = addr6->secid;
> }
>
> - ret_val = security_secid_to_secctx(secid, &secctx, &secctx_len);
> + lsmblob_init(&le, secid);
> + ret_val = security_secid_to_secctx(&le, &secctx, &secctx_len);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 4676f5bb16ae..4145adf55a22 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -100,6 +100,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> struct audit_buffer *audit_buf;
> char *secctx;
> u32 secctx_len;
> + struct lsmblob le;
>
> if (audit_enabled == AUDIT_OFF)
> return NULL;
> @@ -112,10 +113,9 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> from_kuid(&init_user_ns, audit_info->loginuid),
> audit_info->sessionid);
>
> + lsmblob_init(&le, audit_info->secid);
> if (audit_info->secid != 0 &&
> - security_secid_to_secctx(audit_info->secid,
> - &secctx,
> - &secctx_len) == 0) {
> + security_secid_to_secctx(&le, &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " subj=%s", secctx);
> security_release_secctx(secctx, secctx_len);
> }
> diff --git a/security/security.c b/security/security.c
> index 44927bf13d32..561a41eccbd9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1987,10 +1987,18 @@ int security_ismaclabel(const char *name)
> }
> EXPORT_SYMBOL(security_ismaclabel);
>
> -int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> +int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen)
> {
> - return call_int_hook(secid_to_secctx, -EOPNOTSUPP, secid, secdata,
> - seclen);
> + struct security_hook_list *hp;
> + int rc;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> + rc = hp->hook.secid_to_secctx(l->secid[hp->slot],
> + secdata, seclen);
> + if (rc != 0)
> + return rc;
> + }
> + return 0;
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 13/25] LSM: Use lsmblob in security_cred_getsecid
From: Kees Cook @ 2019-06-19 5:11 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-14-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:39PM -0700, Casey Schaufler wrote:
> Change the security_cred_getsecid() interface to fill in a
> lsmblob instead of a u32 secid. The associated data elements
> in the audit sub-system are changed from a secid to a lsmblob
> to accomodate multiple possible LSM audit users.
Cool, cool. I'm digging the clean conversions all the way through this
patch. I didn't see patch 14/25 for some reason, though...
-Kees
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 2 +-
> kernel/audit.c | 14 +++++-------
> kernel/audit.h | 5 +++--
> kernel/auditsc.c | 37 +++++++++++--------------------
> security/integrity/ima/ima_main.c | 6 ++---
> security/security.c | 9 +++++---
> 6 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index b28d4f9c7714..07a239292e02 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -383,7 +383,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> void security_cred_free(struct cred *cred);
> int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
> void security_transfer_creds(struct cred *new, const struct cred *old);
> -void security_cred_getsecid(const struct cred *c, u32 *secid);
> +void security_cred_getsecid(const struct cred *c, struct lsmblob *l);
> int security_kernel_act_as(struct cred *new, struct lsmblob *l);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3ab2a1c0ba61..a52f8772477f 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -137,7 +137,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> /* The identity of the user shutting down the audit system. */
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> -u32 audit_sig_sid = 0;
> +struct lsmblob audit_sig_lsm;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1429,23 +1429,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> }
> case AUDIT_SIGNAL_INFO:
> len = 0;
> - if (audit_sig_sid) {
> - struct lsmblob le;
> -
> - lsmblob_init(&le, audit_sig_sid);
> - err = security_secid_to_secctx(&le, &ctx, &len);
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> + err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> + &len);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (audit_sig_sid)
> + if (lsmblob_is_set(&audit_sig_lsm))
> security_release_secctx(ctx, len);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> - if (audit_sig_sid) {
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 958d5b8fc1b3..29e29c6f4afb 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -22,6 +22,7 @@
> #include <linux/fs.h>
> #include <linux/audit.h>
> #include <linux/skbuff.h>
> +#include <linux/security.h>
> #include <uapi/linux/mqueue.h>
> #include <linux/tty.h>
>
> @@ -147,7 +148,7 @@ struct audit_context {
> kuid_t target_auid;
> kuid_t target_uid;
> unsigned int target_sessionid;
> - u32 target_sid;
> + struct lsmblob target_lsm;
> char target_comm[TASK_COMM_LEN];
>
> struct audit_tree_refs *trees, *first_trees;
> @@ -338,7 +339,7 @@ extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
>
> extern pid_t audit_sig_pid;
> extern kuid_t audit_sig_uid;
> -extern u32 audit_sig_sid;
> +extern struct lsmblob audit_sig_lsm;
>
> extern int audit_filter(int msgtype, unsigned int listtype);
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cdd1dd9e0eec..ebdd7eab9247 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -112,7 +112,7 @@ struct audit_aux_data_pids {
> kuid_t target_auid[AUDIT_AUX_PIDS];
> kuid_t target_uid[AUDIT_AUX_PIDS];
> unsigned int target_sessionid[AUDIT_AUX_PIDS];
> - u32 target_sid[AUDIT_AUX_PIDS];
> + struct lsmblob target_lsm[AUDIT_AUX_PIDS];
> char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> int pid_count;
> };
> @@ -937,14 +937,14 @@ static inline void audit_free_context(struct audit_context *context)
> }
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> - kuid_t auid, kuid_t uid, unsigned int sessionid,
> - u32 sid, char *comm)
> + kuid_t auid, kuid_t uid,
> + unsigned int sessionid,
> + struct lsmblob *l, char *comm)
> {
> struct audit_buffer *ab;
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> - struct lsmblob le;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> if (!ab)
> @@ -953,9 +953,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> - if (sid) {
> - lsmblob_init(&le, sid);
> - if (security_secid_to_secctx(&le, &ctx, &len)) {
> + if (lsmblob_is_set(l)) {
> + if (security_secid_to_secctx(l, &ctx, &len)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> @@ -1525,7 +1524,7 @@ static void audit_log_exit(void)
> axs->target_auid[i],
> axs->target_uid[i],
> axs->target_sessionid[i],
> - axs->target_sid[i],
> + &axs->target_lsm[i],
> axs->target_comm[i]))
> call_panic = 1;
> }
> @@ -1534,7 +1533,7 @@ static void audit_log_exit(void)
> audit_log_pid_context(context, context->target_pid,
> context->target_auid, context->target_uid,
> context->target_sessionid,
> - context->target_sid, context->target_comm))
> + &context->target_lsm, context->target_comm))
> call_panic = 1;
>
> if (context->pwd.dentry && context->pwd.mnt) {
> @@ -1711,7 +1710,7 @@ void __audit_syscall_exit(int success, long return_code)
> context->aux = NULL;
> context->aux_pids = NULL;
> context->target_pid = 0;
> - context->target_sid = 0;
> + lsmblob_init(&context->target_lsm, 0);
> context->sockaddr_len = 0;
> context->type = 0;
> context->fds[0] = -1;
> @@ -2365,15 +2364,12 @@ int __audit_sockaddr(int len, void *a)
> void __audit_ptrace(struct task_struct *t)
> {
> struct audit_context *context = audit_context();
> - struct lsmblob le;
>
> context->target_pid = task_tgid_nr(t);
> context->target_auid = audit_get_loginuid(t);
> context->target_uid = task_uid(t);
> context->target_sessionid = audit_get_sessionid(t);
> - security_task_getsecid(t, &le);
> - /* scaffolding - until target_sid is converted */
> - context->target_sid = le.secid[1];
> + security_task_getsecid(t, &context->target_lsm);
> memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> }
>
> @@ -2390,7 +2386,6 @@ int audit_signal_info(int sig, struct task_struct *t)
> struct audit_aux_data_pids *axp;
> struct audit_context *ctx = audit_context();
> kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
> - struct lsmblob le;
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> @@ -2401,9 +2396,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> - security_task_getsecid(current, &le);
> - /* scaffolding until audit_sig_sid is converted */
> - audit_sig_sid = le.secid[1];
> + security_task_getsecid(current, &audit_sig_lsm);
> }
>
> if (!audit_signals || audit_dummy_context())
> @@ -2416,9 +2409,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> ctx->target_auid = audit_get_loginuid(t);
> ctx->target_uid = t_uid;
> ctx->target_sessionid = audit_get_sessionid(t);
> - security_task_getsecid(t, &le);
> - /* scaffolding until target_sid is converted */
> - ctx->target_sid = le.secid[1];
> + security_task_getsecid(t, &ctx->target_lsm);
> memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> return 0;
> }
> @@ -2439,9 +2430,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> axp->target_auid[axp->pid_count] = audit_get_loginuid(t);
> axp->target_uid[axp->pid_count] = t_uid;
> axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> - security_task_getsecid(t, &le);
> - /* scaffolding until target_sid is converted */
> - axp->target_sid[axp->pid_count] = le.secid[1];
> + security_task_getsecid(t, &axp->target_lsm[axp->pid_count]);
> memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> axp->pid_count++;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 99870a6286a9..9959d7cbe42e 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -363,7 +363,6 @@ int ima_file_mmap(struct file *file, unsigned long prot)
> int ima_bprm_check(struct linux_binprm *bprm)
> {
> int ret;
> - u32 secid;
> struct lsmblob le;
>
> security_task_getsecid(current, &le);
> @@ -373,8 +372,9 @@ int ima_bprm_check(struct linux_binprm *bprm)
> if (ret)
> return ret;
>
> - security_cred_getsecid(bprm->cred, &secid);
> - return process_measurement(bprm->file, bprm->cred, secid, NULL, 0,
> + security_cred_getsecid(bprm->cred, &le);
> + /* scaffolding until process_measurement changes */
> + return process_measurement(bprm->file, bprm->cred, le.secid[1], NULL, 0,
> MAY_EXEC, CREDS_CHECK);
> }
>
> diff --git a/security/security.c b/security/security.c
> index e82994667263..46f6cf21d33c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1638,10 +1638,13 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
> call_void_hook(cred_transfer, new, old);
> }
>
> -void security_cred_getsecid(const struct cred *c, u32 *secid)
> +void security_cred_getsecid(const struct cred *c, struct lsmblob *l)
> {
> - *secid = 0;
> - call_void_hook(cred_getsecid, c, secid);
> + struct security_hook_list *hp;
> +
> + lsmblob_init(l, 0);
> + hlist_for_each_entry(hp, &security_hook_heads.cred_getsecid, list)
> + hp->hook.cred_getsecid(c, &l->secid[hp->slot]);
> }
> EXPORT_SYMBOL(security_cred_getsecid);
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 00/25] LSM: Module stacking for AppArmor
From: Kees Cook @ 2019-06-19 5:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-1-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:26PM -0700, Casey Schaufler wrote:
> Patches 0004-0014 replace system use of a "secid" with
> a structure "lsmblob" containing information from the
> security modules to be held and reused later. At this
> point lsmblob contains an array of u32 secids, one "slot"
> for each of the security modules compiled into the
> kernel that used secids. A "slot" is allocated when
> a security module registers a hook for one of the interfaces
> that uses a secid or a security context. The infrastructure
> is changed to use the slot number to pass the correct
> secid to or from the security module hooks.
I found 14/25 in your git tree. Very satisfying to see all the
scaffolding vanish for process_measurement() :)
I like this progression in 4-14; I find it much much easier to review.
My only complaint is the variable names. I think I'd prefer "blob" over
"le" or "l", which are both contain very little information about what
they are.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 15/25] LSM: Specify which LSM to display
From: Kees Cook @ 2019-06-19 5:28 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-16-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:41PM -0700, Casey Schaufler wrote:
> Create a new entry "display" in /proc/.../attr for controlling
> which LSM security information is displayed for a process.
> The name of an active LSM that supplies hooks for human readable
> data may be written to "display" to set the value. The name of
> the LSM currently in use can be read from "display".
> At this point there can only be one LSM capable of display
> active.
>
> This affects /proc/.../attr/current and SO_PEERSEC.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/proc/base.c | 1 +
> security/security.c | 108 +++++++++++++++++++++++++++++++++++---------
> 2 files changed, 88 insertions(+), 21 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ddef482f1334..7bf70e041315 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "fscreate", 0666),
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> + ATTR(NULL, "display", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/security/security.c b/security/security.c
> index 46f6cf21d33c..9cfdc664239e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
>
> char *lsm_names;
> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
> + .lbs_task = sizeof(int),
> +};
This needs some comments. I know what's happening here only because I
understand very well how the blob sizing works. :) Perhaps:
.lbs_task = sizeof(int), /* storage for selected "display" LSM */
>
> /* Boot-time LSM user choice */
> static __initdata const char *chosen_lsm_order;
> @@ -578,6 +580,8 @@ int lsm_inode_alloc(struct inode *inode)
> */
> static int lsm_task_alloc(struct task_struct *task)
> {
> + int *display;
> +
> if (blob_sizes.lbs_task == 0) {
> task->security = NULL;
> return 0;
> @@ -586,6 +590,10 @@ static int lsm_task_alloc(struct task_struct *task)
> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
> if (task->security == NULL)
> return -ENOMEM;
> +
> + display = task->security;
> + *display = LSMDATA_INVALID;
Similarly I think a comment here would be nice. "Initialize currently
selected "display" LSM to unselected" or something.
Also: "int" is okay here for now, but if the LSM infrastructure wants to
do more like this we'll want to convert to a struct of some kind at the
start of the lbs_task.
> +
> return 0;
> }
>
> @@ -1574,14 +1582,27 @@ int security_file_open(struct file *file)
>
> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> {
> + int *odisplay = current->security;
> + int *ndisplay;
> int rc = lsm_task_alloc(task);
>
> - if (rc)
> + if (unlikely(rc))
> return rc;
> +
> rc = call_int_hook(task_alloc, 0, task, clone_flags);
> - if (unlikely(rc))
> + if (unlikely(rc)) {
> security_task_free(task);
> - return rc;
> + return rc;
> + }
> +
> + ndisplay = task->security;
> + if (ndisplay == NULL)
> + return 0;
> +
> + if (odisplay != NULL)
Perhaps merge these into "if (ndisplay && odisplay)" to drop the early
return 0 check?
> + *ndisplay = *odisplay;
> +
> + return 0;
> }
>
> void security_task_free(struct task_struct *task)
> @@ -1967,10 +1988,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + int *display = current->security;
> +
> + if (!strcmp(name, "display")) {
> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
> + list) {
> + if (*display == LSMDATA_INVALID ||
> + hp->slot == *display) {
> + *value = kstrdup(hp->lsm, GFP_KERNEL);
> + if (*value)
> + return strlen(hp->lsm);
> + return -ENOMEM;
> + }
> + }
> + return -EINVAL;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsm))
> continue;
> + if (lsm == NULL && *display != LSMDATA_INVALID &&
> + *display != hp->slot)
> + continue;
> return hp->hook.getprocattr(p, name, value);
> }
> return -EINVAL;
> @@ -1980,10 +2019,27 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size)
> {
> struct security_hook_list *hp;
> + int *display = current->security;
> + int len;
> +
> + if (!strcmp(name, "display")) {
> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
> + list) {
> + len = strlen(hp->lsm);
> + if (size >= len && !strncmp(value, hp->lsm, len)) {
> + *display = hp->slot;
> + return size;
> + }
> + }
> + return -EINVAL;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsm))
> continue;
> + if (lsm == NULL && *display != LSMDATA_INVALID &&
> + *display != hp->slot)
> + continue;
> return hp->hook.setprocattr(name, value, size);
> }
> return -EINVAL;
> @@ -2002,38 +2058,41 @@ EXPORT_SYMBOL(security_ismaclabel);
>
> int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen)
> {
> + int *display = current->security;
> struct security_hook_list *hp;
> - int rc;
>
> - hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> - rc = hp->hook.secid_to_secctx(l->secid[hp->slot],
> - secdata, seclen);
> - if (rc != 0)
> - return rc;
> - }
> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list)
> + if (*display == LSMDATA_INVALID || *display == hp->slot)
> + return hp->hook.secid_to_secctx(l->secid[hp->slot],
> + secdata, seclen);
> return 0;
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
> int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *l)
> {
> + int *display = current->security;
> struct security_hook_list *hp;
> - int rc;
>
> lsmblob_init(l, 0);
> - hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> - rc = hp->hook.secctx_to_secid(secdata, seclen,
> - &l->secid[hp->slot]);
> - if (rc != 0)
> - return rc;
> - }
> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list)
> + if (*display == LSMDATA_INVALID || *display == hp->slot)
> + return hp->hook.secctx_to_secid(secdata, seclen,
> + &l->secid[hp->slot]);
> return 0;
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> void security_release_secctx(char *secdata, u32 seclen)
> {
> - call_void_hook(release_secctx, secdata, seclen);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> + if (*display == LSMDATA_INVALID || *display == hp->slot) {
> + hp->hook.release_secctx(secdata, seclen);
> + return;
> + }
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> @@ -2158,8 +2217,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len)
> {
> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> - optval, optlen, len);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> + list)
> + if (*display == LSMDATA_INVALID || *display == hp->slot)
> + return hp->hook.socket_getpeersec_stream(sock, optval,
> + optlen, len);
> + return -ENOPROTOOPT;
> }
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 16/25] LSM: Ensure the correct LSM context releaser
From: Kees Cook @ 2019-06-19 5:34 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-17-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:42PM -0700, 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.
>
> Change the security_release_secctx() interface to use the
> lsmcontext and call only the appropiate LSM hook. Change
> the callers of security_release_secctx() to provide the
> correct type of data, introducing scaffolding where required.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> drivers/android/binder.c | 10 +++++--
> fs/kernfs/dir.c | 9 ++++--
> fs/kernfs/inode.c | 7 +++--
> fs/nfs/nfs4proc.c | 8 ++++--
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h | 37 +++++++++++++++++++++++--
> include/net/scm.h | 4 ++-
> 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 | 12 +++++---
> security/smack/smack_lsm.c | 14 +++++++---
> 17 files changed, 140 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 9eb790200fba..f11b5ca5bc30 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2876,6 +2876,7 @@ static void binder_transaction(struct binder_proc *proc,
> int t_debug_id = atomic_inc_return(&binder_last_id);
> char *secctx = NULL;
> u32 secctx_sz = 0;
> + struct lsmcontext scaff; /* scaffolding */
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3158,7 +3159,8 @@ static void binder_transaction(struct binder_proc *proc,
> binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> secctx, secctx_sz);
> - security_release_secctx(secctx, secctx_sz);
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> secctx = NULL;
> }
> t->buffer->debug_id = t->debug_id;
> @@ -3479,8 +3481,10 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer->transaction = NULL;
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> - if (secctx)
> - security_release_secctx(secctx, secctx_sz);
> + if (secctx) {
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> + }
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index b84d635567d3..92afad387237 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -532,9 +532,12 @@ void kernfs_put(struct kernfs_node *kn)
> kfree_const(kn->name);
>
> if (kn->iattr) {
> - if (kn->iattr->ia_secdata)
> - security_release_secctx(kn->iattr->ia_secdata,
> - kn->iattr->ia_secdata_len);
> + struct lsmcontext scaff; /* scaffolding */
> + if (kn->iattr->ia_secdata) {
> + lsmcontext_init(&scaff, kn->iattr->ia_secdata,
> + kn->iattr->ia_secdata_len, 0);
> + security_release_secctx(&scaff);
> + }
> simple_xattrs_free(&kn->iattr->xattrs);
> kmem_cache_free(kernfs_iattrs_cache, kn->iattr);
> }
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 0c1fd945ce42..02cde9dac5ee 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -349,6 +349,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> {
> struct kernfs_node *kn = inode->i_private;
> struct kernfs_iattrs *attrs;
> + struct lsmcontext context;
> void *secdata;
> u32 secdata_len = 0;
> int error;
> @@ -368,8 +369,10 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> mutex_unlock(&kernfs_mutex);
>
> - if (secdata)
> - security_release_secctx(secdata, secdata_len);
> + if (secdata) {
> + lsmcontext_init(&context, secdata, secdata_len, 0);
> + security_release_secctx(&context);
> + }
> return error;
> }
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 4dbb0ee23432..af1c0db29c39 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -131,8 +131,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 lsmcontext 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 3de42a729093..bb3db033e144 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2420,6 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> __be32 status;
> int err;
> struct nfs4_acl *acl = NULL;
> + struct lsmcontext scaff; /* scaffolding */
> void *context = NULL;
> int contextlen;
> bool contextsupport = false;
> @@ -2919,8 +2920,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context)
> - security_release_secctx(context, contextlen);
> + if (context) {
> + lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> + security_release_secctx(&scaff);
> + }
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 07a239292e02..8bd4f28ef532 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -76,6 +76,39 @@ enum lsm_event {
> LSM_POLICY_CHANGE,
> };
>
> +/*
> + * 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.
> + */
> +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);
> +}
I worry about the use of the "size" argument (or rather, lack of use).
If all contexts are going to be NUL-terminated strings, there should be
no "size" arg. If not, then "size" (or strneln(context, size)) should
be used instead of strlen().
> +
> /*
> * Data exported by the security modules
> */
> @@ -445,7 +478,7 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *l);
> -void security_release_secctx(char *secdata, u32 seclen);
> +void security_release_secctx(struct lsmcontext *cp);
>
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1236,7 +1269,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 lsmcontext *cp)
> {
> }
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index bcb0f8560cdf..d3e0ac961a11 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -92,6 +92,7 @@ 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)
> {
> + struct lsmcontext context;
> char *secdata;
> u32 seclen;
> int err;
> @@ -101,7 +102,8 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + lsmcontext_init(&context, secdata, seclen, 0);/*scaffolding*/
> + security_release_secctx(&context);
> }
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a52f8772477f..0467b2d284fa 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1193,6 +1193,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_sig_info *sig_data;
> char *ctx = NULL;
> u32 len;
> + struct lsmcontext scaff; /* scaffolding */
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1437,15 +1438,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm))
> - security_release_secctx(ctx, len);
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> + }
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> memcpy(sig_data->ctx, ctx, len);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2074,6 +2078,7 @@ int audit_log_task_context(struct audit_buffer *ab)
> unsigned len;
> int error;
> struct lsmblob le;
> + struct lsmcontext scaff; /* scaffolding */
>
> security_task_getsecid(current, &le);
> if (!lsmblob_is_set(&le))
> @@ -2087,7 +2092,8 @@ int audit_log_task_context(struct audit_buffer *ab)
> }
>
> audit_log_format(ab, " subj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ebdd7eab9247..917e7550767a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -942,6 +942,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *l, char *comm)
> {
> struct audit_buffer *ab;
> + struct lsmcontext lsmcxt;
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> @@ -959,7 +960,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> rc = 1;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> + security_release_secctx(&lsmcxt);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1171,6 +1173,7 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> + struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1203,7 +1206,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) {
> @@ -1350,6 +1354,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> char *ctx = NULL;
> u32 len;
> struct lsmblob le;
> + struct lsmcontext lsmcxt;
>
> lsmblob_init(&le, n->osid);
> if (security_secid_to_secctx(&le, &ctx, &len)) {
> @@ -1358,7 +1363,8 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> *call_panic = 2;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> + security_release_secctx(&lsmcxt);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index e05f4ef68bd8..7834c357b60b 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,6 +130,7 @@ 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)
> {
> + struct lsmcontext context;
> struct lsmblob lb;
> char *secdata;
> u32 seclen;
> @@ -144,7 +145,8 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> return;
>
> put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
>
> 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 6098b586da07..93f308b5845d 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -331,6 +331,7 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> int len, ret;
> char *secctx;
> struct lsmblob le;
> + struct lsmcontext context;
>
> lsmblob_init(&le, ct->secmark);
> ret = security_secid_to_secctx(&le, &secctx, &len);
> @@ -348,7 +349,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>
> ret = 0;
> nla_put_failure:
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> return ret;
> }
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 6e6fb1f9f6ba..0bde6a4426e3 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -176,6 +176,7 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> u32 len;
> char *secctx;
> struct lsmblob le;
> + struct lsmcontext context;
>
> lsmblob_init(&le, ct->secmark);
> ret = security_secid_to_secctx(&le, &secctx, &len);
> @@ -184,7 +185,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>
> seq_printf(s, "secctx=%s ", secctx);
>
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
> #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 105018d19318..ba767bdd1a9a 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -399,6 +399,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> + struct lsmcontext scaff; /* scaffolding */
> char *secdata = NULL;
> u32 seclen = 0;
>
> @@ -629,8 +630,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:
> @@ -638,8 +641,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 57e0f81a2ec5..2f8c7415b6ff 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -387,6 +387,7 @@ int netlbl_unlhsh_add(struct net *net,
> struct net_device *dev;
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> + struct lsmcontext context;
> char *secctx = NULL;
> u32 secctx_len;
> struct lsmblob le;
> @@ -457,7 +458,9 @@ int netlbl_unlhsh_add(struct net *net,
> &secctx,
> &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -488,6 +491,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct netlbl_unlhsh_addr4 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob le;
> @@ -516,7 +520,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> security_secid_to_secctx(&le,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -553,6 +559,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct netlbl_unlhsh_addr6 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob le;
> @@ -580,7 +587,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> security_secid_to_secctx(&le,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -1094,6 +1102,7 @@ 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 lsmcontext context;
> void *data;
> u32 secid;
> char *secctx;
> @@ -1161,7 +1170,9 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> NLBL_UNLABEL_A_SECCTX,
> secctx_len,
> secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
>
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 4145adf55a22..fba861c4ffbb 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -98,6 +98,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> struct netlbl_audit *audit_info)
> {
> struct audit_buffer *audit_buf;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob le;
> @@ -117,7 +118,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> if (audit_info->secid != 0 &&
> security_secid_to_secctx(&le, &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " subj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_release_secctx(&context);
> }
>
> return audit_buf;
> diff --git a/security/security.c b/security/security.c
> index 9cfdc664239e..d25c099b46d1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -458,6 +458,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> &security_hook_heads.socket_getpeersec_dgram ||
> hooks[i].head == &security_hook_heads.secctx_to_secid ||
> hooks[i].head == &security_hook_heads.secid_to_secctx ||
> + hooks[i].head == &security_hook_heads.release_secctx ||
> hooks[i].head == &security_hook_heads.ipc_getsecid ||
> hooks[i].head == &security_hook_heads.task_getsecid ||
> hooks[i].head == &security_hook_heads.inode_getsecid ||
> @@ -2083,16 +2084,19 @@ int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *l)
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> -void security_release_secctx(char *secdata, u32 seclen)
> +void security_release_secctx(struct lsmcontext *cp)
> {
> - int *display = current->security;
> struct security_hook_list *hp;
>
> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> - if (*display == LSMDATA_INVALID || *display == hp->slot) {
> - hp->hook.release_secctx(secdata, seclen);
> + if (cp->slot == hp->slot) {
> + hp->hook.release_secctx(cp->context, cp->len);
> + lsmcontext_init(cp, NULL, 0, 0);
> return;
> }
> +
> + pr_warn("%s context \"%s\" from slot %d not released\n", __func__,
> + cp->context, cp->slot);
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e9560b078efe..3834b751d1e9 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4439,11 +4439,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)
> {
> @@ -4683,6 +4688,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),
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 17/25] LSM: Use lsmcontext in security_secid_to_secctx
From: Kees Cook @ 2019-06-19 5:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-18-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:43PM -0700, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext 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.
I like these because they're obviously sensible changes: swap two args
for one. :)
-Kees
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> drivers/android/binder.c | 24 ++++++---------
> include/linux/security.h | 4 +--
> include/net/scm.h | 8 ++---
> kernel/audit.c | 29 +++++++-----------
> kernel/auditsc.c | 31 +++++++------------
> net/ipv4/ip_sockglue.c | 7 ++---
> net/netfilter/nf_conntrack_netlink.c | 14 +++++----
> net/netfilter/nf_conntrack_standalone.c | 7 ++---
> net/netfilter/nfnetlink_queue.c | 5 +++-
> net/netlabel/netlabel_unlabeled.c | 40 ++++++++-----------------
> net/netlabel/netlabel_user.c | 7 ++---
> security/security.c | 9 +++---
> 12 files changed, 71 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f11b5ca5bc30..aad7cdc8137f 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2874,9 +2874,7 @@ static void binder_transaction(struct binder_proc *proc,
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> int t_debug_id = atomic_inc_return(&binder_last_id);
> - char *secctx = NULL;
> - u32 secctx_sz = 0;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext lsmctx;
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3123,14 +3121,14 @@ static void binder_transaction(struct binder_proc *proc,
> struct lsmblob le;
>
> security_task_getsecid(proc->tsk, &le);
> - ret = security_secid_to_secctx(&le, &secctx, &secctx_sz);
> + ret = security_secid_to_secctx(&le, &lsmctx);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> return_error_line = __LINE__;
> goto err_get_secctx_failed;
> }
> - extra_buffers_size += ALIGN(secctx_sz, sizeof(u64));
> + extra_buffers_size += ALIGN(lsmctx.len, sizeof(u64));
> }
>
> trace_binder_transaction(reply, t, target_node);
> @@ -3149,19 +3147,17 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer = NULL;
> goto err_binder_alloc_buf_failed;
> }
> - if (secctx) {
> + if (lsmctx.context) {
> 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 = (uintptr_t)t->buffer->user_data + buf_offset;
> binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> - secctx, secctx_sz);
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - secctx = NULL;
> + lsmctx.context, lsmctx.len);
> + security_release_secctx(&lsmctx);
> }
> t->buffer->debug_id = t->debug_id;
> t->buffer->transaction = t;
> @@ -3481,10 +3477,8 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer->transaction = NULL;
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> - if (secctx) {
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmctx.context)
> + security_release_secctx(&lsmctx);
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8bd4f28ef532..1fd87e80656f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -475,7 +475,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen);
> +int security_secid_to_secctx(struct lsmblob *l, struct lsmcontext *cp);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *l);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1257,7 +1257,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *l,
> - char **secdata, u32 *seclen)
> + struct lsmcontext *cp)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index d3e0ac961a11..4a6ad8caf423 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -93,16 +93,14 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> {
> struct lsmcontext context;
> - char *secdata;
> - u32 seclen;
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(&scm->lsmblob, &secdata, &seclen);
> + err = security_secid_to_secctx(&scm->lsmblob, &context);
>
> if (!err) {
> - put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - lsmcontext_init(&context, secdata, seclen, 0);/*scaffolding*/
> + put_cmsg(msg, SOL_SOCKET, SCM_SECURITY,
> + context.len, context.context);
> security_release_secctx(&context);
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 0467b2d284fa..33a08f49b52e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1191,9 +1191,8 @@ 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 lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context;
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1431,25 +1430,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> - &len);
> + err = security_secid_to_secctx(&audit_sig_lsm,
> + &context);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm)) {
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmblob_is_set(&audit_sig_lsm))
> + security_release_secctx(&context);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - memcpy(sig_data->ctx, ctx, len);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + memcpy(sig_data->ctx, context.context, context.len);
> + security_release_secctx(&context);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2074,26 +2070,23 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>
> int audit_log_task_context(struct audit_buffer *ab)
> {
> - char *ctx = NULL;
> - unsigned len;
> int error;
> struct lsmblob le;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context;
>
> security_task_getsecid(current, &le);
> if (!lsmblob_is_set(&le))
> return 0;
>
> - error = security_secid_to_secctx(&le, &ctx, &len);
> + error = security_secid_to_secctx(&le, &context);
> if (error) {
> if (error != -EINVAL)
> goto error_path;
> return 0;
> }
>
> - audit_log_format(ab, " subj=%s", ctx);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + audit_log_format(ab, " subj=%s", context.context);
> + security_release_secctx(&context);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 917e7550767a..847c1d59212d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -942,9 +942,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *l, char *comm)
> {
> struct audit_buffer *ab;
> - struct lsmcontext lsmcxt;
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmctx;
> int rc = 0;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> @@ -955,13 +953,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 (lsmblob_is_set(l)) {
> - if (security_secid_to_secctx(l, &ctx, &len)) {
> + if (security_secid_to_secctx(l, &lsmctx)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1173,7 +1170,6 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> - struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1197,16 +1193,14 @@ static void show_special(struct audit_context *context, int *call_panic)
> from_kgid(&init_user_ns, context->ipc.gid),
> context->ipc.mode);
> if (osid) {
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmcxt;
> struct lsmblob le;
> lsmblob_init(&le, osid);
> - if (security_secid_to_secctx(&le, &ctx, &len)) {
> + if (security_secid_to_secctx(&le, &lsmcxt)) {
> audit_log_format(ab, " osid=%u", osid);
> *call_panic = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0);
> + audit_log_format(ab, " obj=%s", lsmcxt.context);
> security_release_secctx(&lsmcxt);
> }
> }
> @@ -1351,20 +1345,17 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> MAJOR(n->rdev),
> MINOR(n->rdev));
> if (n->osid != 0) {
> - char *ctx = NULL;
> - u32 len;
> struct lsmblob le;
> - struct lsmcontext lsmcxt;
> + struct lsmcontext lsmctx;
>
> lsmblob_init(&le, n->osid);
> - if (security_secid_to_secctx(&le, &ctx, &len)) {
> + if (security_secid_to_secctx(&le, &lsmctx)) {
> audit_log_format(ab, " osid=%u", n->osid);
> if (call_panic)
> *call_panic = 2;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 7834c357b60b..80ae0c5a1301 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -132,20 +132,17 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> {
> struct lsmcontext context;
> struct lsmblob lb;
> - char *secdata;
> - u32 seclen;
> int err;
>
> err = security_socket_getpeersec_dgram(NULL, skb, &lb);
> if (err)
> return;
>
> - err = security_secid_to_secctx(&lb, &secdata, &seclen);
> + err = security_secid_to_secctx(&lb, &context);
> if (err)
> return;
>
> - put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + put_cmsg(msg, SOL_IP, SCM_SECURITY, context.len, context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 93f308b5845d..8d9943b925d7 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -328,13 +328,12 @@ 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;
> + int ret;
> struct lsmblob le;
> struct lsmcontext context;
>
> lsmblob_init(&le, ct->secmark);
> - ret = security_secid_to_secctx(&le, &secctx, &len);
> + ret = security_secid_to_secctx(&le, &context);
> if (ret)
> return 0;
>
> @@ -343,13 +342,12 @@ 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, context.context))
> goto nla_put_failure;
> nla_nest_end(skb, nest_secctx);
>
> ret = 0;
> nla_put_failure:
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> return ret;
> }
> @@ -620,12 +618,16 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
> int len, ret;
> struct lsmblob le;
> + struct lsmcontext context;
>
> lsmblob_init(&le, ct->secmark);
> - ret = security_secid_to_secctx(&le, NULL, &len);
> + ret = security_secid_to_secctx(&le, &context);
> if (ret)
> return 0;
>
> + len = context.len;
> + security_release_secctx(&context);
> +
> return nla_total_size(0) /* CTA_SECCTX */
> + nla_total_size(sizeof(char) * len); /* CTA_SECCTX_NAME */
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 0bde6a4426e3..3085a090af7a 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -173,19 +173,16 @@ static void ct_seq_stop(struct seq_file *s, void *v)
> static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> {
> int ret;
> - u32 len;
> - char *secctx;
> struct lsmblob le;
> struct lsmcontext context;
>
> lsmblob_init(&le, ct->secmark);
> - ret = security_secid_to_secctx(&le, &secctx, &len);
> + ret = security_secid_to_secctx(&le, &context);
> if (ret)
> return;
>
> - seq_printf(s, "secctx=%s ", secctx);
> + seq_printf(s, "secctx=%s ", context.context);
>
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> }
> #else
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index ba767bdd1a9a..60948538711b 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -310,6 +310,7 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob le;
> + struct lsmcontext context;
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -318,10 +319,12 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>
> if (skb->secmark) {
> lsmblob_init(&le, skb->secmark);
> - security_secid_to_secctx(&le, secdata, &seclen);
> + security_secid_to_secctx(&le, &context);
> + *secdata = context.context;
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> + seclen = context.len;
> #endif
> return seclen;
> }
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 2f8c7415b6ff..35e7d595f2b9 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -388,8 +388,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - char *secctx = NULL;
> - u32 secctx_len;
> struct lsmblob le;
>
> if (addr_len != sizeof(struct in_addr) &&
> @@ -454,12 +452,9 @@ int netlbl_unlhsh_add(struct net *net,
> rcu_read_unlock();
> if (audit_buf != NULL) {
> lsmblob_init(&le, secid);
> - if (security_secid_to_secctx(&le,
> - &secctx,
> - &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + if (security_secid_to_secctx(&le, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> @@ -492,8 +487,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob le;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -517,11 +510,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> if (entry != NULL)
> lsmblob_init(&le, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&le,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&le, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -560,8 +551,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob le;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -584,10 +573,9 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> if (entry != NULL)
> lsmblob_init(&le, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&le,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&le, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -1105,8 +1093,6 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct lsmcontext context;
> void *data;
> u32 secid;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob le;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> @@ -1163,15 +1149,13 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> }
>
> lsmblob_init(&le, secid);
> - ret_val = security_secid_to_secctx(&le, &secctx, &secctx_len);
> + ret_val = security_secid_to_secctx(&le, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> NLBL_UNLABEL_A_SECCTX,
> - secctx_len,
> - secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + context.len,
> + context.context);
> security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index fba861c4ffbb..52d1ea2bd605 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -99,8 +99,6 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> {
> struct audit_buffer *audit_buf;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob le;
>
> if (audit_enabled == AUDIT_OFF)
> @@ -116,9 +114,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>
> lsmblob_init(&le, audit_info->secid);
> if (audit_info->secid != 0 &&
> - security_secid_to_secctx(&le, &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " subj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_secid_to_secctx(&le, &context) == 0) {
> + audit_log_format(audit_buf, " subj=%s", context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/security/security.c b/security/security.c
> index d25c099b46d1..2ea810fc4a45 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -457,7 +457,6 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> hooks[i].head ==
> &security_hook_heads.socket_getpeersec_dgram ||
> hooks[i].head == &security_hook_heads.secctx_to_secid ||
> - hooks[i].head == &security_hook_heads.secid_to_secctx ||
> hooks[i].head == &security_hook_heads.release_secctx ||
> hooks[i].head == &security_hook_heads.ipc_getsecid ||
> hooks[i].head == &security_hook_heads.task_getsecid ||
> @@ -2057,15 +2056,17 @@ int security_ismaclabel(const char *name)
> }
> EXPORT_SYMBOL(security_ismaclabel);
>
> -int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen)
> +int security_secid_to_secctx(struct lsmblob *l, struct lsmcontext *cp)
> {
> int *display = current->security;
> struct security_hook_list *hp;
>
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list)
> - if (*display == LSMDATA_INVALID || *display == hp->slot)
> + if (*display == LSMDATA_INVALID || *display == hp->slot) {
> + cp->slot = hp->slot;
> return hp->hook.secid_to_secctx(l->secid[hp->slot],
> - secdata, seclen);
> + &cp->context, &cp->len);
> + }
> return 0;
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 18/25] LSM: Use lsmcontext in security_dentry_init_security
From: Kees Cook @ 2019-06-19 5:41 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-19-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:44PM -0700, Casey Schaufler wrote:
> Chance the security_dentry_init_security() interface to
> fill an lsmcontext structure instead of a void * data area
> and a length. The lone caller of this interface is NFS4,
> which may make copies of the data using its own mechanisms.
> A rework of the nfs4 code to use the lsmcontext properly
> is a significant project, so the coward's way out is taken,
> and the lsmcontext data from security_dentry_init_security()
> is copied, then released directly.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
> include/linux/security.h | 7 +++----
> security/security.c | 20 ++++++++++++++++----
> 3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index af1c0db29c39..952f805965bb 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -113,6 +113,7 @@ static inline struct nfs4_label *
> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> struct iattr *sattr, struct nfs4_label *label)
> {
> + struct lsmcontext context;
> int err;
>
> if (label == NULL)
> @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> return NULL;
>
> err = security_dentry_init_security(dentry, sattr->ia_mode,
> - &dentry->d_name, (void **)&label->label, &label->len);
> - if (err == 0)
> - return label;
> + &dentry->d_name, &context);
> +
> + if (err)
> + return NULL;
> +
> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
I think this is wrong: for NUL-terminated strings, "context.len" isn't
currently including the NUL byte (it's set to strlen()).
So, if kmemdup() is used here, it means strlen() isn't correct in the
context init helper, it should be using the "size" argument, etc.
> + if (label->label == NULL)
> + label = NULL;
> + else
> + label->len = context.len;
> +
> + security_release_secctx(&context);
> +
> + return label;
>
> - return NULL;
> }
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - struct lsmcontext scaff; /* scaffolding */
> -
> - if (label) {
> - lsmcontext_init(&scaff, label->label, label->len, 0);
> - security_release_secctx(&scaff);
> - }
> + kfree(label->label);
Should label be set to NULL here and len reduced to 0?
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1fd87e80656f..92c4960dd57f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -346,8 +346,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> int security_add_mnt_opt(const char *option, const char *val,
> int len, void **mnt_opts);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + struct lsmcontext *ctx);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -718,8 +718,7 @@ static inline void security_inode_free(struct inode *inode)
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> - void **ctx,
> - u32 *ctxlen)
> + struct lsmcontext *ctx)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 2ea810fc4a45..23d8049ec0c1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> * secid in the lsmblob structure.
> */
> if (hooks[i].head == &security_hook_heads.audit_rule_match ||
> + hooks[i].head ==
> + &security_hook_heads.dentry_init_security ||
> hooks[i].head == &security_hook_heads.kernel_act_as ||
> hooks[i].head ==
> &security_hook_heads.socket_getpeersec_dgram ||
> @@ -1030,11 +1032,21 @@ void security_inode_free(struct inode *inode)
> }
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + struct lsmcontext *cp)
> {
> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
> + list)
> + if (*display == 0 || *display == hp->slot) {
> + cp->slot = hp->slot;
> + return hp->hook.dentry_init_security(dentry, mode,
> + name, (void **)&cp->context, &cp->len);
> + }
> +
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 24/25] Fix slotted list and getpeersec_d
From: Kees Cook @ 2019-06-19 5:50 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190618230551.7475-25-casey@schaufler-ca.com>
On Tue, Jun 18, 2019 at 04:05:50PM -0700, Casey Schaufler wrote:
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Was this supposed to be folded into patch 4?
-Kees
> ---
> security/security.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/security/security.c b/security/security.c
> index 5a23ccec7c7b..8aca43ab3e81 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -461,6 +461,8 @@ int __init security_add_hooks(struct security_hook_list *hooks, int count,
> hooks[i].head == &security_hook_heads.kernel_act_as ||
> hooks[i].head ==
> &security_hook_heads.socket_getpeersec_dgram ||
> + hooks[i].head == &security_hook_heads.getprocattr ||
> + hooks[i].head == &security_hook_heads.setprocattr ||
> hooks[i].head == &security_hook_heads.secctx_to_secid ||
> hooks[i].head == &security_hook_heads.release_secctx ||
> hooks[i].head == &security_hook_heads.ipc_getsecid ||
> @@ -2269,7 +2271,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> list) {
> rc = hp->hook.socket_getpeersec_dgram(sock, skb,
> &l->secid[hp->slot]);
> - if (rc != 0)
> + if (rc == -ENOPROTOOPT)
> + rc = 0;
> + else if (rc != 0)
> break;
> }
> return rc;
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 04/25] LSM: Create and manage the lsmblob data structure.
From: Kees Cook @ 2019-06-19 6:17 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201906182147.0A592CBB62@keescook>
On Tue, Jun 18, 2019 at 09:52:44PM -0700, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:30PM -0700, Casey Schaufler wrote:
> > When more than one security module is exporting data to
> > audit and networking sub-systems a single 32 bit integer
> > is no longer sufficient to represent the data. Add a
> > structure to be used instead.
> >
> > The lsmblob structure is currently an array of
> > u32 "secids". There is an entry for each of the
> > security modules built into the system that would
> > use secids if active. The system assigns the module
> > a "slot" when it registers hooks. If modules are
> > compiled in but not registered there will be unused
> > slots.
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > ---
> > include/linux/lsm_hooks.h | 1 +
> > include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++
> > security/security.c | 31 ++++++++++++++++++++
> > 3 files changed, 94 insertions(+)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 3fe39abccc8f..4d1ddf1a2aa6 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -2038,6 +2038,7 @@ struct security_hook_list {
> > struct hlist_head *head;
> > union security_list_options hook;
> > char *lsm;
> > + int slot;
> > } __randomize_layout;
>
> Hm, this feels redundant (as does the existing "char *lsm") now that we
> have lsm_info. The place for assigned-at-init value is blob_sizes, which
> hangs off of lsm_info (as does the LSM char *)...
Hm, nevermind. lsm_info is __initdata. I will ponder a way to refactor
this in the future. For now, just leave slot in here with char *lsm.
--
Kees Cook
^ permalink raw reply
* [PATCH 00/10] keys: Miscellany [ver #3]
From: David Howells @ 2019-06-19 13:18 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: Mat Martineau, Eric Biggers, James Morris, dhowells,
linux-security-module, linux-kernel
Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window:
(1) Fix a bunch of warnings from sparse, including missing RCU bits and
kdoc-function argument mismatches
(2) Implement a keyctl to allow a key to be moved from one keyring to
another, with the option of prohibiting key replacement in the
destination keyring.
(3) Grant Link permission to possessors of request_key_auth tokens so that
upcall servicing daemons can more easily arrange things such that only
the necessary auth key is passed to the actual service program, and
not all the auth keys a daemon might possesss.
(4) Improvement in lookup_user_key().
(5) Implement a keyctl to allow keyrings subsystem capabilities to be
queried.
The patches can be found on the following branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-misc
The keyutils next branch has commits to make available, document and test
the move-key and capabilities code:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/log/?h=next
Changes:
V3:
(*) Made the capabilities array unsigned char[] rather than unsigned int[].
V2:
(*) Fixed lock ordering bug in KEYCTL_MOVE.
(*) Added improvement patch from Eric.
(*) Added capabilities patch.
David
---
David Howells (9):
keys: sparse: Fix key_fs[ug]id_changed()
keys: sparse: Fix incorrect RCU accesses
keys: sparse: Fix kdoc mismatches
keys: Change keyring_serialise_link_sem to a mutex
keys: Break bits out of key_unlink()
keys: Hoist locking out of __key_link_begin()
keys: Add a keyctl to move a key between keyrings
keys: Grant Link permission to possessers of request_key auth keys
keys: Add capability-checking keyctl function
Eric Biggers (1):
keys: Reuse keyring_index_key::desc_len in lookup_user_key()
Documentation/security/keys/core.rst | 21 +++
include/linux/key.h | 13 +-
include/uapi/linux/keyctl.h | 17 ++
kernel/cred.c | 4
security/keys/compat.c | 6 +
security/keys/internal.h | 7 +
security/keys/key.c | 27 +++
security/keys/keyctl.c | 90 +++++++++++
security/keys/keyring.c | 278 ++++++++++++++++++++++++++++------
security/keys/process_keys.c | 26 +--
security/keys/request_key.c | 9 +
security/keys/request_key_auth.c | 4
12 files changed, 418 insertions(+), 84 deletions(-)
^ permalink raw reply
* [PATCH 01/10] keys: sparse: Fix key_fs[ug]id_changed() [ver #3]
From: David Howells @ 2019-06-19 13:18 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: James Morris, dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
accesses of tsk->cred, which is marked __rcu.
Fix this by passing the new cred struct to these functions from
commit_creds() rather than the task pointer.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
include/linux/key.h | 8 ++++----
kernel/cred.c | 4 ++--
security/keys/process_keys.c | 22 ++++++++++------------
3 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..1f09aad1c98c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -402,8 +402,8 @@ extern struct ctl_table key_sysctls[];
* the userspace interface
*/
extern int install_thread_keyring_to_cred(struct cred *cred);
-extern void key_fsuid_changed(struct task_struct *tsk);
-extern void key_fsgid_changed(struct task_struct *tsk);
+extern void key_fsuid_changed(struct cred *new_cred);
+extern void key_fsgid_changed(struct cred *new_cred);
extern void key_init(void);
#else /* CONFIG_KEYS */
@@ -418,8 +418,8 @@ extern void key_init(void);
#define make_key_ref(k, p) NULL
#define key_ref_to_ptr(k) NULL
#define is_key_possessed(k) 0
-#define key_fsuid_changed(t) do { } while(0)
-#define key_fsgid_changed(t) do { } while(0)
+#define key_fsuid_changed(c) do { } while(0)
+#define key_fsgid_changed(c) do { } while(0)
#define key_init() do { } while(0)
#endif /* CONFIG_KEYS */
diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..3bd40de9e192 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -455,9 +455,9 @@ int commit_creds(struct cred *new)
/* alter the thread keyring */
if (!uid_eq(new->fsuid, old->fsuid))
- key_fsuid_changed(task);
+ key_fsuid_changed(new);
if (!gid_eq(new->fsgid, old->fsgid))
- key_fsgid_changed(task);
+ key_fsgid_changed(new);
/* do it
* RLIMIT_NPROC limits on user->processes have already been checked
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..ba5d3172cafe 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -293,28 +293,26 @@ static int install_session_keyring(struct key *keyring)
/*
* Handle the fsuid changing.
*/
-void key_fsuid_changed(struct task_struct *tsk)
+void key_fsuid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->uid = tsk->cred->fsuid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->uid = new_cred->fsuid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
/*
* Handle the fsgid changing.
*/
-void key_fsgid_changed(struct task_struct *tsk)
+void key_fsgid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->gid = tsk->cred->fsgid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->gid = new_cred->fsgid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
^ permalink raw reply related
* [PATCH 02/10] keys: sparse: Fix incorrect RCU accesses [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: James Morris, dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Fix a pair of accesses that should be using RCU protection.
rcu_dereference_protected() is needed to access task_struct::real_parent.
current_cred() should be used to access current->cred.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/keyctl.c | 3 ++-
security/keys/request_key_auth.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..0f947bcbad46 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1524,7 +1524,8 @@ long keyctl_session_to_parent(void)
ret = -EPERM;
oldwork = NULL;
- parent = me->real_parent;
+ parent = rcu_dereference_protected(me->real_parent,
+ lockdep_is_held(&tasklist_lock));
/* the parent mustn't be init and mustn't be a kernel thread */
if (parent->pid <= 1 || !parent->mm)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..572c7a60473a 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -152,7 +152,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
struct key *dest_keyring)
{
struct request_key_auth *rka, *irka;
- const struct cred *cred = current->cred;
+ const struct cred *cred = current_cred();
struct key *authkey = NULL;
char desc[20];
int ret = -ENOMEM;
^ permalink raw reply related
* [PATCH 03/10] keys: sparse: Fix kdoc mismatches [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: James Morris, Mat Martineau, dhowells, linux-security-module,
linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Fix some kdoc argument description mismatches reported by sparse and give
keyring_restrict() a description.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
security/keys/keyring.c | 10 +++++++---
security/keys/request_key.c | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..5b218b270598 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -520,7 +520,7 @@ EXPORT_SYMBOL(keyring_alloc);
* @keyring: The keyring being added to.
* @type: The type of key being added.
* @payload: The payload of the key intended to be added.
- * @data: Additional data for evaluating restriction.
+ * @restriction_key: Keys providing additional data for evaluating restriction.
*
* Reject the addition of any links to a keyring. It can be overridden by
* passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
@@ -976,9 +976,13 @@ static bool keyring_detect_restriction_cycle(const struct key *dest_keyring,
/**
* keyring_restrict - Look up and apply a restriction to a keyring
- *
- * @keyring: The keyring to be restricted
+ * @keyring_ref: The keyring to be restricted
+ * @type: The key type that will provide the restriction checker.
* @restriction: The restriction options to apply to the keyring
+ *
+ * Look up a keyring and apply a restriction to it. The restriction is managed
+ * by the specific key type, but can be configured by the options specified in
+ * the restriction string.
*/
int keyring_restrict(key_ref_t keyring_ref, const char *type,
const char *restriction)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..1f234b019437 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -24,7 +24,7 @@
/**
* complete_request_key - Complete the construction of a key.
- * @auth_key: The authorisation key.
+ * @authkey: The authorisation key.
* @error: The success or failute of the construction.
*
* Complete the attempt to construct a key. The key will be negated
^ permalink raw reply related
* [PATCH 04/10] keys: Change keyring_serialise_link_sem to a mutex [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Change keyring_serialise_link_sem to a mutex as it's only ever
write-locked.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5b218b270598..ca6694ba1773 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -100,7 +100,7 @@ EXPORT_SYMBOL(key_type_keyring);
* Semaphore to serialise link/link calls to prevent two link calls in parallel
* introducing a cycle.
*/
-static DECLARE_RWSEM(keyring_serialise_link_sem);
+static DEFINE_MUTEX(keyring_serialise_link_lock);
/*
* Publish the name of a keyring so that it can be found by name (if it has
@@ -1206,7 +1206,7 @@ int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
__acquires(&keyring->sem)
- __acquires(&keyring_serialise_link_sem)
+ __acquires(&keyring_serialise_link_lock)
{
struct assoc_array_edit *edit;
int ret;
@@ -1228,7 +1228,7 @@ int __key_link_begin(struct key *keyring,
/* serialise link/link calls to prevent parallel calls causing a cycle
* when linking two keyring in opposite orders */
if (index_key->type == &key_type_keyring)
- down_write(&keyring_serialise_link_sem);
+ mutex_lock(&keyring_serialise_link_lock);
/* Create an edit script that will insert/replace the key in the
* keyring tree.
@@ -1260,7 +1260,7 @@ int __key_link_begin(struct key *keyring,
assoc_array_cancel_edit(edit);
error_sem:
if (index_key->type == &key_type_keyring)
- up_write(&keyring_serialise_link_sem);
+ mutex_unlock(&keyring_serialise_link_lock);
error_krsem:
up_write(&keyring->sem);
kleave(" = %d", ret);
@@ -1307,13 +1307,13 @@ void __key_link_end(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit *edit)
__releases(&keyring->sem)
- __releases(&keyring_serialise_link_sem)
+ __releases(&keyring_serialise_link_lock)
{
BUG_ON(index_key->type == NULL);
kenter("%d,%s,", keyring->serial, index_key->type->name);
if (index_key->type == &key_type_keyring)
- up_write(&keyring_serialise_link_sem);
+ mutex_unlock(&keyring_serialise_link_lock);
if (edit) {
if (!edit->dead_leaf) {
^ permalink raw reply related
* [PATCH 05/10] keys: Break bits out of key_unlink() [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Break bits out of key_unlink() into helper functions so that they can be
used in implementing key_move().
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 88 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 67 insertions(+), 21 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index ca6694ba1773..6990c7761eaa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1382,6 +1382,65 @@ int key_link(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_link);
+/*
+ * Lock a keyring for unlink.
+ */
+static int __key_unlink_lock(struct key *keyring)
+ __acquires(&keyring->sem)
+{
+ if (keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ down_write(&keyring->sem);
+ return 0;
+}
+
+/*
+ * Begin the process of unlinking a key from a keyring.
+ */
+static int __key_unlink_begin(struct key *keyring, struct key *key,
+ struct assoc_array_edit **_edit)
+{
+ struct assoc_array_edit *edit;
+
+ BUG_ON(*_edit != NULL);
+
+ edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
+ &key->index_key);
+ if (IS_ERR(edit))
+ return PTR_ERR(edit);
+
+ if (!edit)
+ return -ENOENT;
+
+ *_edit = edit;
+ return 0;
+}
+
+/*
+ * Apply an unlink change.
+ */
+static void __key_unlink(struct key *keyring, struct key *key,
+ struct assoc_array_edit **_edit)
+{
+ assoc_array_apply_edit(*_edit);
+ *_edit = NULL;
+ key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
+}
+
+/*
+ * Finish unlinking a key from to a keyring.
+ */
+static void __key_unlink_end(struct key *keyring,
+ struct key *key,
+ struct assoc_array_edit *edit)
+ __releases(&keyring->sem)
+{
+ if (edit)
+ assoc_array_cancel_edit(edit);
+ up_write(&keyring->sem);
+}
+
/**
* key_unlink - Unlink the first link to a key from a keyring.
* @keyring: The keyring to remove the link from.
@@ -1401,33 +1460,20 @@ EXPORT_SYMBOL(key_link);
*/
int key_unlink(struct key *keyring, struct key *key)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret;
key_check(keyring);
key_check(key);
- if (keyring->type != &key_type_keyring)
- return -ENOTDIR;
-
- down_write(&keyring->sem);
-
- edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
- &key->index_key);
- if (IS_ERR(edit)) {
- ret = PTR_ERR(edit);
- goto error;
- }
- ret = -ENOENT;
- if (edit == NULL)
- goto error;
-
- assoc_array_apply_edit(edit);
- key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
- ret = 0;
+ ret = __key_unlink_lock(keyring);
+ if (ret < 0)
+ return ret;
-error:
- up_write(&keyring->sem);
+ ret = __key_unlink_begin(keyring, key, &edit);
+ if (ret == 0)
+ __key_unlink(keyring, key, &edit);
+ __key_unlink_end(keyring, key, edit);
return ret;
}
EXPORT_SYMBOL(key_unlink);
^ permalink raw reply related
* [PATCH 06/10] keys: Hoist locking out of __key_link_begin() [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Hoist the locking of out of __key_link_begin() and into its callers. This
is necessary to allow the upcoming key_move() operation to correctly order
taking of the source keyring semaphore, the destination keyring semaphore
and the keyring serialisation lock.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/internal.h | 2 +
security/keys/key.c | 27 ++++++++++++---
security/keys/keyring.c | 78 ++++++++++++++++++++++++++-----------------
security/keys/request_key.c | 7 +++-
4 files changed, 76 insertions(+), 38 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..25cdd0cbdc06 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,6 +93,8 @@ extern wait_queue_head_t request_key_conswq;
extern struct key_type *key_type_lookup(const char *type);
extern void key_type_put(struct key_type *ktype);
+extern int __key_link_lock(struct key *keyring,
+ const struct keyring_index_key *index_key);
extern int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..bba71acec886 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -500,7 +500,7 @@ int key_instantiate_and_link(struct key *key,
struct key *authkey)
{
struct key_preparsed_payload prep;
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret;
memset(&prep, 0, sizeof(prep));
@@ -515,10 +515,14 @@ int key_instantiate_and_link(struct key *key,
}
if (keyring) {
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_lock(keyring, &key->index_key);
if (ret < 0)
goto error;
+ ret = __key_link_begin(keyring, &key->index_key, &edit);
+ if (ret < 0)
+ goto error_link_end;
+
if (keyring->restrict_link && keyring->restrict_link->check) {
struct key_restriction *keyres = keyring->restrict_link;
@@ -570,7 +574,7 @@ int key_reject_and_link(struct key *key,
struct key *keyring,
struct key *authkey)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret, awaken, link_ret = 0;
key_check(key);
@@ -583,7 +587,12 @@ int key_reject_and_link(struct key *key,
if (keyring->restrict_link)
return -EPERM;
- link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ link_ret = __key_link_lock(keyring, &key->index_key);
+ if (link_ret == 0) {
+ link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ if (link_ret < 0)
+ __key_link_end(keyring, &key->index_key, edit);
+ }
}
mutex_lock(&key_construction_mutex);
@@ -810,7 +819,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
.description = description,
};
struct key_preparsed_payload prep;
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
const struct cred *cred = current_cred();
struct key *keyring, *key = NULL;
key_ref_t key_ref;
@@ -860,12 +869,18 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
index_key.desc_len = strlen(index_key.description);
- ret = __key_link_begin(keyring, &index_key, &edit);
+ ret = __key_link_lock(keyring, &index_key);
if (ret < 0) {
key_ref = ERR_PTR(ret);
goto error_free_prep;
}
+ ret = __key_link_begin(keyring, &index_key, &edit);
+ if (ret < 0) {
+ key_ref = ERR_PTR(ret);
+ goto error_link_end;
+ }
+
if (restrict_link && restrict_link->check) {
ret = restrict_link->check(keyring, index_key.type,
&prep.payload, restrict_link->key);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 6990c7761eaa..12acad3db6cf 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1199,14 +1199,34 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
return PTR_ERR(ctx.result) == -EAGAIN ? 0 : PTR_ERR(ctx.result);
}
+/*
+ * Lock keyring for link.
+ */
+int __key_link_lock(struct key *keyring,
+ const struct keyring_index_key *index_key)
+ __acquires(&keyring->sem)
+ __acquires(&keyring_serialise_link_lock)
+{
+ if (keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ down_write(&keyring->sem);
+
+ /* Serialise link/link calls to prevent parallel calls causing a cycle
+ * when linking two keyring in opposite orders.
+ */
+ if (index_key->type == &key_type_keyring)
+ mutex_lock(&keyring_serialise_link_lock);
+
+ return 0;
+}
+
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
- __acquires(&keyring->sem)
- __acquires(&keyring_serialise_link_lock)
{
struct assoc_array_edit *edit;
int ret;
@@ -1215,20 +1235,13 @@ int __key_link_begin(struct key *keyring,
keyring->serial, index_key->type->name, index_key->description);
BUG_ON(index_key->desc_len == 0);
+ BUG_ON(*_edit != NULL);
- if (keyring->type != &key_type_keyring)
- return -ENOTDIR;
-
- down_write(&keyring->sem);
+ *_edit = NULL;
ret = -EKEYREVOKED;
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
- goto error_krsem;
-
- /* serialise link/link calls to prevent parallel calls causing a cycle
- * when linking two keyring in opposite orders */
- if (index_key->type == &key_type_keyring)
- mutex_lock(&keyring_serialise_link_lock);
+ goto error;
/* Create an edit script that will insert/replace the key in the
* keyring tree.
@@ -1239,7 +1252,7 @@ int __key_link_begin(struct key *keyring,
NULL);
if (IS_ERR(edit)) {
ret = PTR_ERR(edit);
- goto error_sem;
+ goto error;
}
/* If we're not replacing a link in-place then we're going to need some
@@ -1258,11 +1271,7 @@ int __key_link_begin(struct key *keyring,
error_cancel:
assoc_array_cancel_edit(edit);
-error_sem:
- if (index_key->type == &key_type_keyring)
- mutex_unlock(&keyring_serialise_link_lock);
-error_krsem:
- up_write(&keyring->sem);
+error:
kleave(" = %d", ret);
return ret;
}
@@ -1312,9 +1321,6 @@ void __key_link_end(struct key *keyring,
BUG_ON(index_key->type == NULL);
kenter("%d,%s,", keyring->serial, index_key->type->name);
- if (index_key->type == &key_type_keyring)
- mutex_unlock(&keyring_serialise_link_lock);
-
if (edit) {
if (!edit->dead_leaf) {
key_payload_reserve(keyring,
@@ -1323,6 +1329,9 @@ void __key_link_end(struct key *keyring,
assoc_array_cancel_edit(edit);
}
up_write(&keyring->sem);
+
+ if (index_key->type == &key_type_keyring)
+ mutex_unlock(&keyring_serialise_link_lock);
}
/*
@@ -1358,7 +1367,7 @@ static int __key_link_check_restriction(struct key *keyring, struct key *key)
*/
int key_link(struct key *keyring, struct key *key)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret;
kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
@@ -1366,17 +1375,24 @@ int key_link(struct key *keyring, struct key *key)
key_check(keyring);
key_check(key);
+ ret = __key_link_lock(keyring, &key->index_key);
+ if (ret < 0)
+ goto error;
+
ret = __key_link_begin(keyring, &key->index_key, &edit);
- if (ret == 0) {
- kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
- ret = __key_link_check_restriction(keyring, key);
- if (ret == 0)
- ret = __key_link_check_live_key(keyring, key);
- if (ret == 0)
- __key_link(key, &edit);
- __key_link_end(keyring, &key->index_key, edit);
- }
+ if (ret < 0)
+ goto error_end;
+
+ kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
+ ret = __key_link_check_restriction(keyring, key);
+ if (ret == 0)
+ ret = __key_link_check_live_key(keyring, key);
+ if (ret == 0)
+ __key_link(key, &edit);
+error_end:
+ __key_link_end(keyring, &key->index_key, edit);
+error:
kleave(" = %d {%d,%d}", ret, keyring->serial, refcount_read(&keyring->usage));
return ret;
}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..857da65e1940 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -343,7 +343,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
struct key_user *user,
struct key **_key)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
struct key *key;
key_perm_t perm;
key_ref_t key_ref;
@@ -372,6 +372,9 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
if (dest_keyring) {
+ ret = __key_link_lock(dest_keyring, &ctx->index_key);
+ if (ret < 0)
+ goto link_lock_failed;
ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
if (ret < 0)
goto link_prealloc_failed;
@@ -423,6 +426,8 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
return ret;
link_prealloc_failed:
+ __key_link_end(dest_keyring, &ctx->index_key, edit);
+link_lock_failed:
mutex_unlock(&user->cons_lock);
key_put(key);
kleave(" = %d [prelink]", ret);
^ permalink raw reply related
* [PATCH 07/10] keys: Add a keyctl to move a key between keyrings [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Add a keyctl to atomically move a link to a key from one keyring to
another. The key must exist in "from" keyring and a flag can be given to
cause the operation to fail if there's a matching key already in the "to"
keyring.
This can be done with:
keyctl(KEYCTL_MOVE,
key_serial_t key,
key_serial_t from_keyring,
key_serial_t to_keyring,
unsigned int flags);
The key being moved must grant Link permission and both keyrings must grant
Write permission.
flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
displacement of a matching key from the "to" keyring.
Signed-off-by: David Howells <dhowells@redhat.com>
---
Documentation/security/keys/core.rst | 21 +++++++
include/linux/key.h | 5 ++
include/uapi/linux/keyctl.h | 3 +
security/keys/compat.c | 3 +
security/keys/internal.h | 3 +
security/keys/keyctl.c | 52 ++++++++++++++++
security/keys/keyring.c | 108 ++++++++++++++++++++++++++++++++++
7 files changed, 195 insertions(+)
diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 9521c4207f01..823d29bf44f7 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -577,6 +577,27 @@ The keyctl syscall functions are:
added.
+ * Move a key from one keyring to another::
+
+ long keyctl(KEYCTL_MOVE,
+ key_serial_t id,
+ key_serial_t from_ring_id,
+ key_serial_t to_ring_id,
+ unsigned int flags);
+
+ Move the key specified by "id" from the keyring specified by
+ "from_ring_id" to the keyring specified by "to_ring_id". If the two
+ keyrings are the same, nothing is done.
+
+ "flags" can have KEYCTL_MOVE_EXCL set in it to cause the operation to fail
+ with EEXIST if a matching key exists in the destination keyring, otherwise
+ such a key will be replaced.
+
+ A process must have link permission on the key for this function to be
+ successful and write permission on both keyrings. Any errors that can
+ occur from KEYCTL_LINK also apply on the destination keyring here.
+
+
* Unlink a key or keyring from another keyring::
long keyctl(KEYCTL_UNLINK, key_serial_t keyring, key_serial_t key);
diff --git a/include/linux/key.h b/include/linux/key.h
index 1f09aad1c98c..612e1cf84049 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -310,6 +310,11 @@ extern int key_update(key_ref_t key,
extern int key_link(struct key *keyring,
struct key *key);
+extern int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags);
+
extern int key_unlink(struct key *keyring,
struct key *key);
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index f45ee0f69c0c..fd9fb11b312b 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -67,6 +67,7 @@
#define KEYCTL_PKEY_SIGN 27 /* Create a public key signature */
#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key signature */
#define KEYCTL_RESTRICT_KEYRING 29 /* Restrict keys allowed to link to a keyring */
+#define KEYCTL_MOVE 30 /* Move keys between keyrings */
/* keyctl structures */
struct keyctl_dh_params {
@@ -112,4 +113,6 @@ struct keyctl_pkey_params {
__u32 __spare[7];
};
+#define KEYCTL_MOVE_EXCL 0x00000001 /* Do not displace from the to-keyring */
+
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 9482df601dc3..b326bc4f84d7 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3),
compat_ptr(arg4), compat_ptr(arg5));
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 25cdd0cbdc06..b54a58c025ae 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -95,6 +95,8 @@ extern void key_type_put(struct key_type *ktype);
extern int __key_link_lock(struct key *keyring,
const struct keyring_index_key *index_key);
+extern int __key_move_lock(struct key *l_keyring, struct key *u_keyring,
+ const struct keyring_index_key *index_key);
extern int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
@@ -217,6 +219,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t);
extern long keyctl_revoke_key(key_serial_t);
extern long keyctl_keyring_clear(key_serial_t);
extern long keyctl_keyring_link(key_serial_t, key_serial_t);
+extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int);
extern long keyctl_keyring_unlink(key_serial_t, key_serial_t);
extern long keyctl_describe_key(key_serial_t, char __user *, size_t);
extern long keyctl_keyring_search(key_serial_t, const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0f947bcbad46..bbfe7d92d41c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -572,6 +572,52 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
return ret;
}
+/*
+ * Move a link to a key from one keyring to another, displacing any matching
+ * key from the destination keyring.
+ *
+ * The key must grant the caller Link permission and both keyrings must grant
+ * the caller Write permission. There must also be a link in the from keyring
+ * to the key. If both keyrings are the same, nothing is done.
+ *
+ * If successful, 0 will be returned.
+ */
+long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
+ key_serial_t to_ringid, unsigned int flags)
+{
+ key_ref_t key_ref, from_ref, to_ref;
+ long ret;
+
+ if (flags & ~KEYCTL_MOVE_EXCL)
+ return -EINVAL;
+
+ key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
+
+ from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+ if (IS_ERR(from_ref)) {
+ ret = PTR_ERR(from_ref);
+ goto error2;
+ }
+
+ to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ if (IS_ERR(to_ref)) {
+ ret = PTR_ERR(to_ref);
+ goto error3;
+ }
+
+ ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
+ key_ref_to_ptr(to_ref), flags);
+
+ key_ref_put(to_ref);
+error3:
+ key_ref_put(from_ref);
+error2:
+ key_ref_put(key_ref);
+ return ret;
+}
+
/*
* Return a description of a key to userspace.
*
@@ -1772,6 +1818,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
(const void __user *)arg4,
(const void __user *)arg5);
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move((key_serial_t)arg2,
+ (key_serial_t)arg3,
+ (key_serial_t)arg4,
+ (unsigned int)arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 12acad3db6cf..67066bb58b83 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1221,6 +1221,40 @@ int __key_link_lock(struct key *keyring,
return 0;
}
+/*
+ * Lock keyrings for move (link/unlink combination).
+ */
+int __key_move_lock(struct key *l_keyring, struct key *u_keyring,
+ const struct keyring_index_key *index_key)
+ __acquires(&l_keyring->sem)
+ __acquires(&u_keyring->sem)
+ __acquires(&keyring_serialise_link_lock)
+{
+ if (l_keyring->type != &key_type_keyring ||
+ u_keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ /* We have to be very careful here to take the keyring locks in the
+ * right order, lest we open ourselves to deadlocking against another
+ * move operation.
+ */
+ if (l_keyring < u_keyring) {
+ down_write(&l_keyring->sem);
+ down_write_nested(&u_keyring->sem, 1);
+ } else {
+ down_write(&u_keyring->sem);
+ down_write_nested(&l_keyring->sem, 1);
+ }
+
+ /* Serialise link/link calls to prevent parallel calls causing a cycle
+ * when linking two keyring in opposite orders.
+ */
+ if (index_key->type == &key_type_keyring)
+ mutex_lock(&keyring_serialise_link_lock);
+
+ return 0;
+}
+
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
@@ -1494,6 +1528,80 @@ int key_unlink(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_unlink);
+/**
+ * key_move - Move a key from one keyring to another
+ * @key: The key to move
+ * @from_keyring: The keyring to remove the link from.
+ * @to_keyring: The keyring to make the link in.
+ * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL.
+ *
+ * Make a link in @to_keyring to a key, such that the keyring holds a reference
+ * on that key and the key can potentially be found by searching that keyring
+ * whilst simultaneously removing a link to the key from @from_keyring.
+ *
+ * This function will write-lock both keyring's semaphores and will consume
+ * some of the user's key data quota to hold the link on @to_keyring.
+ *
+ * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring,
+ * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second
+ * keyring is full, -EDQUOT if there is insufficient key data quota remaining
+ * to add another link or -ENOMEM if there's insufficient memory. If
+ * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a
+ * matching key in @to_keyring.
+ *
+ * It is assumed that the caller has checked that it is permitted for a link to
+ * be made (the keyring should have Write permission and the key Link
+ * permission).
+ */
+int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags)
+{
+ struct assoc_array_edit *from_edit = NULL, *to_edit = NULL;
+ int ret;
+
+ kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial);
+
+ if (from_keyring == to_keyring)
+ return 0;
+
+ key_check(key);
+ key_check(from_keyring);
+ key_check(to_keyring);
+
+ ret = __key_move_lock(from_keyring, to_keyring, &key->index_key);
+ if (ret < 0)
+ goto out;
+ ret = __key_unlink_begin(from_keyring, key, &from_edit);
+ if (ret < 0)
+ goto error;
+ ret = __key_link_begin(to_keyring, &key->index_key, &to_edit);
+ if (ret < 0)
+ goto error;
+
+ ret = -EEXIST;
+ if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL))
+ goto error;
+
+ ret = __key_link_check_restriction(to_keyring, key);
+ if (ret < 0)
+ goto error;
+ ret = __key_link_check_live_key(to_keyring, key);
+ if (ret < 0)
+ goto error;
+
+ __key_unlink(from_keyring, key, &from_edit);
+ __key_link(key, &to_edit);
+error:
+ __key_link_end(to_keyring, &key->index_key, to_edit);
+ __key_unlink_end(from_keyring, key, from_edit);
+out:
+ kleave(" = %d", ret);
+ return ret;
+}
+EXPORT_SYMBOL(key_move);
+
/**
* keyring_clear - Clear a keyring
* @keyring: The keyring to clear.
^ permalink raw reply related
* [PATCH 08/10] keys: Grant Link permission to possessers of request_key auth keys [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: James Morris, dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Grant Link permission to the possessers of request_key authentication keys,
thereby allowing a daemon that is servicing upcalls to arrange things such
that only the necessary auth key is passed to the actual service program
and not all the daemon's pending auth keys.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/request_key_auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 572c7a60473a..ec5226557023 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -204,7 +204,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
authkey = key_alloc(&key_type_request_key_auth, desc,
cred->fsuid, cred->fsgid, cred,
- KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+ KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_LINK |
KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
if (IS_ERR(authkey)) {
ret = PTR_ERR(authkey);
^ permalink raw reply related
* [PATCH 09/10] keys: Reuse keyring_index_key::desc_len in lookup_user_key() [ver #3]
From: David Howells @ 2019-06-19 13:19 UTC (permalink / raw)
To: keyrings, ebiggers
Cc: Eric Biggers, James Morris, dhowells, linux-security-module,
linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
From: Eric Biggers <ebiggers@google.com>
When lookup_user_key() checks whether the key is possessed, it should
use the key's existing index_key including the 'desc_len' field, rather
than recomputing the 'desc_len'. This doesn't change the behavior; this
way is just simpler and faster.
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/process_keys.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index ba5d3172cafe..39aaa21462bf 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -688,9 +688,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
key_ref = make_key_ref(key, 0);
/* check to see if we possess the key */
- ctx.index_key.type = key->type;
- ctx.index_key.description = key->description;
- ctx.index_key.desc_len = strlen(key->description);
+ ctx.index_key = key->index_key;
ctx.match_data.raw_data = key;
kdebug("check possessed");
skey_ref = search_process_keyrings(&ctx);
^ permalink raw reply related
* [PATCH 10/10] keys: Add capability-checking keyctl function [ver #3]
From: David Howells @ 2019-06-19 13:20 UTC (permalink / raw)
To: keyrings, ebiggers; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <156095032052.9363.8954337545422131435.stgit@warthog.procyon.org.uk>
Add a keyctl function that requests a set of capability bits to find out
what features are supported.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/uapi/linux/keyctl.h | 14 ++++++++++++++
security/keys/compat.c | 3 +++
security/keys/internal.h | 2 ++
security/keys/keyctl.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 54 insertions(+)
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index fd9fb11b312b..551b5814f53e 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -68,6 +68,7 @@
#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key signature */
#define KEYCTL_RESTRICT_KEYRING 29 /* Restrict keys allowed to link to a keyring */
#define KEYCTL_MOVE 30 /* Move keys between keyrings */
+#define KEYCTL_CAPABILITIES 31 /* Find capabilities of keyrings subsystem */
/* keyctl structures */
struct keyctl_dh_params {
@@ -115,4 +116,17 @@ struct keyctl_pkey_params {
#define KEYCTL_MOVE_EXCL 0x00000001 /* Do not displace from the to-keyring */
+/*
+ * Capabilities flags. The capabilities list is an array of 8-bit integers;
+ * each integer can carry up to 8 flags.
+ */
+#define KEYCTL_CAPS0_CAPABILITIES 0x01 /* KEYCTL_CAPABILITIES supported */
+#define KEYCTL_CAPS0_PERSISTENT_KEYRINGS 0x02 /* Persistent keyrings enabled */
+#define KEYCTL_CAPS0_DIFFIE_HELLMAN 0x04 /* Diffie-Hellman computation enabled */
+#define KEYCTL_CAPS0_PUBLIC_KEY 0x08 /* Public key ops enabled */
+#define KEYCTL_CAPS0_BIG_KEY 0x10 /* big_key-type enabled */
+#define KEYCTL_CAPS0_INVALIDATE 0x20 /* KEYCTL_INVALIDATE supported */
+#define KEYCTL_CAPS0_RESTRICT_KEYRING 0x40 /* KEYCTL_RESTRICT_KEYRING supported */
+#define KEYCTL_CAPS0_MOVE 0x80 /* KEYCTL_MOVE supported */
+
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index b326bc4f84d7..a53e30da20c5 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -162,6 +162,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
case KEYCTL_MOVE:
return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+ case KEYCTL_CAPABILITIES:
+ return keyctl_capabilities(compat_ptr(arg2), arg3);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index b54a58c025ae..d04bff631227 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -329,6 +329,8 @@ static inline long keyctl_pkey_e_d_s(int op,
}
#endif
+extern long keyctl_capabilities(unsigned char __user *_buffer, size_t buflen);
+
/*
* Debugging key validation
*/
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index bbfe7d92d41c..9f418e66f067 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -30,6 +30,18 @@
#define KEY_MAX_DESC_SIZE 4096
+static const unsigned char keyrings_capabilities[1] = {
+ [0] = (KEYCTL_CAPS0_CAPABILITIES |
+ (IS_ENABLED(CONFIG_PERSISTENT_KEYRINGS) ? KEYCTL_CAPS0_PERSISTENT_KEYRINGS : 0) |
+ (IS_ENABLED(CONFIG_KEY_DH_OPERATIONS) ? KEYCTL_CAPS0_DIFFIE_HELLMAN : 0) |
+ (IS_ENABLED(CONFIG_ASYMMETRIC_KEY_TYPE) ? KEYCTL_CAPS0_PUBLIC_KEY : 0) |
+ (IS_ENABLED(CONFIG_BIG_KEYS) ? KEYCTL_CAPS0_BIG_KEY : 0) |
+ KEYCTL_CAPS0_INVALIDATE |
+ KEYCTL_CAPS0_RESTRICT_KEYRING |
+ KEYCTL_CAPS0_MOVE
+ ),
+};
+
static int key_get_type_from_user(char *type,
const char __user *_type,
unsigned len)
@@ -1678,6 +1690,26 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type,
return ret;
}
+/*
+ * Get keyrings subsystem capabilities.
+ */
+long keyctl_capabilities(unsigned char __user *_buffer, size_t buflen)
+{
+ size_t size = buflen;
+
+ if (size > 0) {
+ if (size > sizeof(keyrings_capabilities))
+ size = sizeof(keyrings_capabilities);
+ if (copy_to_user(_buffer, keyrings_capabilities, size) != 0)
+ return -EFAULT;
+ if (size < buflen &&
+ clear_user(_buffer + size, buflen - size) != 0)
+ return -EFAULT;
+ }
+
+ return sizeof(keyrings_capabilities);
+}
+
/*
* The key control system call
*/
@@ -1824,6 +1856,9 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
(key_serial_t)arg4,
(unsigned int)arg5);
+ case KEYCTL_CAPABILITIES:
+ return keyctl_capabilities((unsigned char __user *)arg2, (size_t)arg3);
+
default:
return -EOPNOTSUPP;
}
^ permalink raw reply related
* Re: [PATCH 2/3] IMA:Define a new template field buf
From: Mimi Zohar @ 2019-06-19 13:54 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: roberto.sassu
In-Reply-To: <20190617183507.14160-3-prsriva02@gmail.com>
On Mon, 2019-06-17 at 11:35 -0700, Prakhar Srivastava wrote:
> A buffer(kexec boot command line arguments) measured into IMA
> measuremnt list cannot be appraised, without already being
> aware of the buffer contents. Since hashes are non-reversible,
> raw buffer is needed for validation or regenerating hash for
> appraisal/attestation.
>
> Add support to store/read the buffer contents in HEX.
> The kexec cmdline hash is stored in the "d-ng" field of the
> template data,it can be verified using
> sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
> grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
>
> - Add two new fields to ima_event_data to hold the buf and
> buf_len [Suggested by Roberto]
> - Add a new temaplte field 'buf' to be used to store/read
> the buffer data.[Suggested by Mimi]
> - Updated process_buffer_meaurement to add the buffer to
> ima_event_data. process_buffer_measurement added in
> "Define a new IMA hook to measure the boot command line
> arguments"
> - Add a new template policy name ima-buf to represent
> 'd-ng|n-ng|buf'
>
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: James Morris <jamorris@linux.microsoft.com>
Thanks, looking much better.
>
> /* IMA template field data definition */
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index ea7d8cbf712f..83ca99d65e4b 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -140,7 +140,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> struct ima_template_entry *entry;
> struct inode *inode = file_inode(file);
> struct ima_event_data event_data = {iint, file, filename, NULL, 0,
> - cause};
> + cause, NULL, 0};
> int violation = 1;
> int result;
>
> @@ -296,7 +296,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
> struct inode *inode = file_inode(file);
> struct ima_template_entry *entry;
> struct ima_event_data event_data = {iint, file, filename, xattr_value,
> - xattr_len, NULL};
> + xattr_len, NULL, NULL, 0};
> int violation = 0;
>
> if (iint->measured_pcrs & (0x1 << pcr))
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 993d0f1915ff..c8591406c0e2 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> struct ima_template_entry *entry;
> struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
> - NULL, 0, NULL};
> + NULL, 0, NULL, NULL, 0};
> int result = -ENOMEM;
> int violation = 0;
> struct {
>
These changes shouldn't be necessary. Please rebase these patches on
top of the latest next-queued-testing branch (git remote update). "IMA: support for per
policy rule template formats" is still changing.
Minor nit. When re-posting the patches please update the patch titles
so that there is a space between the subsystem name and the patch
title (eg. "ima: define ...").
Mimi
^ permalink raw reply
* Re: [PATCH v2 15/25] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-06-19 15:33 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182127.073A9D7@keescook>
On 6/18/2019 9:33 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:41PM -0700, Casey Schaufler wrote:
>> Create a new entry "display" in /proc/.../attr for controlling
>> which LSM security information is displayed for a process.
>> The name of an active LSM that supplies hooks for human readable
>> data may be written to "display" to set the value. The name of
>> the LSM currently in use can be read from "display".
>> At this point there can only be one LSM capable of display
>> active.
>>
>> This affects /proc/.../attr/current and SO_PEERSEC.
> What happened to creating /proc/$pid/lsm/$lsm_name/current for "modern"
> LSM libraries to start using (instead of possibly fighting over the
> /proc/$pid/attr/display)?
Smack already has it and the mechanics are available for
anyone who wants to use it. John says that AppArmor is moving
away from using the attr interfaces. Stephen and Paul have been
silent on the topic, but my assumption is that the SELinux
attitude is "I had them first, it's not my problem". When
we get around to creating liblsm, with the "real" LSM user
space APIs it will probably drive the addition of more
/proc/$pid/lsm/$lsm_name directories.
> (Obviously "display" is needed for "old"
> libraries, and I'm fine with it.)
Yes, and we can expect some distos to cling to the
old libraries for a looooong time.
> Similarly, is there something that can be done for SO_PEERSEC that
> doesn't require using "display" for "modern" libraries?
Yes, and I made sure the implementation could
accommodate that. It would be easy to add a "display"
that would use the much discussed $lsm1="a",$lsm2="b"
format. I didn't include it because without a liblsm to
use it it's just clutter.
>
> -Kees
>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> fs/proc/base.c | 1 +
>> security/security.c | 108 +++++++++++++++++++++++++++++++++++---------
>> 2 files changed, 88 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index ddef482f1334..7bf70e041315 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>> ATTR(NULL, "fscreate", 0666),
>> ATTR(NULL, "keycreate", 0666),
>> ATTR(NULL, "sockcreate", 0666),
>> + ATTR(NULL, "display", 0666),
>> #ifdef CONFIG_SECURITY_SMACK
>> DIR("smack", 0555,
>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/security/security.c b/security/security.c
>> index 46f6cf21d33c..9cfdc664239e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache;
>> static struct kmem_cache *lsm_inode_cache;
>>
>> char *lsm_names;
>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>> + .lbs_task = sizeof(int),
>> +};
>>
>> /* Boot-time LSM user choice */
>> static __initdata const char *chosen_lsm_order;
>> @@ -578,6 +580,8 @@ int lsm_inode_alloc(struct inode *inode)
>> */
>> static int lsm_task_alloc(struct task_struct *task)
>> {
>> + int *display;
>> +
>> if (blob_sizes.lbs_task == 0) {
>> task->security = NULL;
>> return 0;
>> @@ -586,6 +590,10 @@ static int lsm_task_alloc(struct task_struct *task)
>> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
>> if (task->security == NULL)
>> return -ENOMEM;
>> +
>> + display = task->security;
>> + *display = LSMDATA_INVALID;
>> +
>> return 0;
>> }
>>
>> @@ -1574,14 +1582,27 @@ int security_file_open(struct file *file)
>>
>> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>> {
>> + int *odisplay = current->security;
>> + int *ndisplay;
>> int rc = lsm_task_alloc(task);
>>
>> - if (rc)
>> + if (unlikely(rc))
>> return rc;
>> +
>> rc = call_int_hook(task_alloc, 0, task, clone_flags);
>> - if (unlikely(rc))
>> + if (unlikely(rc)) {
>> security_task_free(task);
>> - return rc;
>> + return rc;
>> + }
>> +
>> + ndisplay = task->security;
>> + if (ndisplay == NULL)
>> + return 0;
>> +
>> + if (odisplay != NULL)
>> + *ndisplay = *odisplay;
>> +
>> + return 0;
>> }
>>
>> void security_task_free(struct task_struct *task)
>> @@ -1967,10 +1988,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + int *display = current->security;
>> +
>> + if (!strcmp(name, "display")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>> + list) {
>> + if (*display == LSMDATA_INVALID ||
>> + hp->slot == *display) {
>> + *value = kstrdup(hp->lsm, GFP_KERNEL);
>> + if (*value)
>> + return strlen(hp->lsm);
>> + return -ENOMEM;
>> + }
>> + }
>> + return -EINVAL;
>> + }
>>
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMDATA_INVALID &&
>> + *display != hp->slot)
>> + continue;
>> return hp->hook.getprocattr(p, name, value);
>> }
>> return -EINVAL;
>> @@ -1980,10 +2019,27 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> size_t size)
>> {
>> struct security_hook_list *hp;
>> + int *display = current->security;
>> + int len;
>> +
>> + if (!strcmp(name, "display")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>> + list) {
>> + len = strlen(hp->lsm);
>> + if (size >= len && !strncmp(value, hp->lsm, len)) {
>> + *display = hp->slot;
>> + return size;
>> + }
>> + }
>> + return -EINVAL;
>> + }
>>
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMDATA_INVALID &&
>> + *display != hp->slot)
>> + continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>> return -EINVAL;
>> @@ -2002,38 +2058,41 @@ EXPORT_SYMBOL(security_ismaclabel);
>>
>> int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen)
>> {
>> + int *display = current->security;
>> struct security_hook_list *hp;
>> - int rc;
>>
>> - hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
>> - rc = hp->hook.secid_to_secctx(l->secid[hp->slot],
>> - secdata, seclen);
>> - if (rc != 0)
>> - return rc;
>> - }
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.secid_to_secctx(l->secid[hp->slot],
>> + secdata, seclen);
>> return 0;
>> }
>> EXPORT_SYMBOL(security_secid_to_secctx);
>>
>> int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *l)
>> {
>> + int *display = current->security;
>> struct security_hook_list *hp;
>> - int rc;
>>
>> lsmblob_init(l, 0);
>> - hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
>> - rc = hp->hook.secctx_to_secid(secdata, seclen,
>> - &l->secid[hp->slot]);
>> - if (rc != 0)
>> - return rc;
>> - }
>> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.secctx_to_secid(secdata, seclen,
>> + &l->secid[hp->slot]);
>> return 0;
>> }
>> EXPORT_SYMBOL(security_secctx_to_secid);
>>
>> void security_release_secctx(char *secdata, u32 seclen)
>> {
>> - call_void_hook(release_secctx, secdata, seclen);
>> + int *display = current->security;
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot) {
>> + hp->hook.release_secctx(secdata, seclen);
>> + return;
>> + }
>> }
>> EXPORT_SYMBOL(security_release_secctx);
>>
>> @@ -2158,8 +2217,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>> int __user *optlen, unsigned len)
>> {
>> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
>> - optval, optlen, len);
>> + int *display = current->security;
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> + list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.socket_getpeersec_stream(sock, optval,
>> + optlen, len);
>> + return -ENOPROTOOPT;
>> }
>>
>> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> --
>> 2.20.1
>>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox