[PATCH v12 14/25] LSM: Ensure the correct LSM context releaser

[Stephen Smalley]

On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
> 
> Reviewed-by: Kees Cook <keescook at chromium.org>
> Reviewed-by: John Johansen <john.johansen at canonical.com>
> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> cc: linux-integrity at vger.kernel.org
> cc: netdev at vger.kernel.org
> ---
>   drivers/android/binder.c                | 10 +++++--
>   fs/ceph/xattr.c                         |  6 +++-
>   fs/nfs/nfs4proc.c                       |  8 +++--
>   fs/nfsd/nfs4xdr.c                       |  7 +++--
>   include/linux/security.h                | 39 +++++++++++++++++++++++--
>   include/net/scm.h                       |  5 +++-
>   kernel/audit.c                          | 14 ++++++---
>   kernel/auditsc.c                        | 12 ++++++--
>   net/ipv4/ip_sockglue.c                  |  4 ++-
>   net/netfilter/nf_conntrack_netlink.c    |  4 ++-
>   net/netfilter/nf_conntrack_standalone.c |  4 ++-
>   net/netfilter/nfnetlink_queue.c         | 13 ++++++---
>   net/netlabel/netlabel_unlabeled.c       | 19 +++++++++---
>   net/netlabel/netlabel_user.c            |  4 ++-
>   security/security.c                     | 18 ++++++++----
>   security/smack/smack_lsm.c              | 14 ++++++---
>   16 files changed, 141 insertions(+), 40 deletions(-)
> 

> diff --git a/include/linux/security.h b/include/linux/security.h
> index d12b5e828b8d..597d9802b89b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -128,6 +128,41 @@ enum lockdown_reason {
>   	LOCKDOWN_CONFIDENTIALITY_MAX,
>   };
>   
> +/*
> + * A "security context" is the text representation of
> + * the information used by LSMs.
> + * This structure contains the string, its length, and which LSM
> + * it is useful for.
> + */
> +struct lsmcontext {
> +	char	*context;	/* Provided by the module */
> +	u32	len;
> +	int	slot;		/* Identifies the module */
> +};
> +
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> +				   u32 size, int slot)
> +{
> +	cp->slot = slot;
> +	cp->context = context;
> +
> +	if (context == NULL || size == 0)
> +		cp->len = 0;
> +	else
> +		cp->len = strlen(context);
> +}

Why do you recompute the length instead of just storing size? Aside from 
being less efficient, it may also be incorrect; SELinux-generated 
contexts include the terminating NUL byte.
> diff --git a/security/security.c b/security/security.c
> index aaac748e4d83..6310ca7e84ed 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2144,17 +2144,23 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
>   }
>   EXPORT_SYMBOL(security_secctx_to_secid);
>   
> -void security_release_secctx(char *secdata, u32 seclen)
> +void security_release_secctx(struct lsmcontext *cp)
>   {
>   	struct security_hook_list *hp;
> -	int *display = current->security;
> +	bool found = false;
>   
>   	hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> -		if (display == NULL || *display == LSMBLOB_INVALID ||
> -		    *display == hp->lsmid->slot) {
> -			hp->hook.release_secctx(secdata, seclen);
> -			return;
> +		if (cp->slot == hp->lsmid->slot) {
> +			hp->hook.release_secctx(cp->context, cp->len);
> +			found = true;
> +			break;
>   		}
> +
> +	memset(cp, 0, sizeof(*cp));
> +
> +	if (!found)
> +		pr_warn("%s context \"%s\" from slot %d not released\n",
> +			__func__, cp->context, cp->slot);

Not sure we need this warning but regardless, you cleared cp before the 
pr_warn() so the output won't be very useful.

>   }
>   EXPORT_SYMBOL(security_release_secctx);
>   
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 9737ead06b39..8e960f82bf3f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4482,11 +4482,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>   	return 0;
>   }
>   
> -/*
> - * There used to be a smack_release_secctx hook
> - * that did nothing back when hooks were in a vector.
> - * Now that there's a list such a hook adds cost.
> +/**
> + * smack_release_secctx - do everything necessary to free a context
> + * @secdata: Unused
> + * @seclen: Unused
> + *
> + * Do nothing but hold a slot in the hooks list.
>    */
> +static void smack_release_secctx(char *secdata, u32 seclen)
> +{
> +}
>   
>   static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>   {
> @@ -4729,6 +4734,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
>   	LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
>   	LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
> +	LSM_HOOK_INIT(release_secctx, smack_release_secctx),
>   	LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
>   	LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
>   	LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
> 

Is this just to avoid the warning above?  If so, I'd just get rid of the 
warning instead.