Re: [PATCH v2 04/25] LSM: Create and manage the lsmblob data structure.

[Kees Cook <keescook@chromium.org>]

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