[PATCH v3 04/24] LSM: Create and manage the lsmblob data structure.

From: John Johansen <john.johansen@canonical.com>
To: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: keescook@chromium.org, penguin-kernel@i-love.sakura.ne.jp,
	paul@paul-moore.com, sds@tycho.nsa.gov
Subject: [PATCH v3 04/24] LSM: Create and manage the lsmblob data structure.
Date: Mon, 24 Jun 2019 11:38:19 -0700	[thread overview]
Message-ID: <0510f853-e80f-1683-6391-eb4c4eba576c@canonical.com> (raw)
In-Reply-To: <20190621185233.6766-5-casey@schaufler-ca.com>

On 6/21/19 11:52 AM, 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>

See below

> ---
>  include/linux/lsm_hooks.h |  1 +
>  include/linux/security.h  | 62 +++++++++++++++++++++++++++++++++++++++
>  security/security.c       | 36 +++++++++++++++++++++++
>  3 files changed, 99 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;
>

I agree with Kees, *lsm and slot should be moved to the lsm_info
and a lsm_info * should take their place here. This will help
with slot assignment below. But this can come as a separate
patch later.

>  /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 49f2685324b0..0aa9417a5762 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 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[LSMBLOB_ENTRIES];
> +};
> +
> +#define LSMBLOB_INVALID	-1
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.
> + * @blob: 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 *blob, u32 secid)
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		blob->secid[i] = secid;
> +}
> +
> +/**
> + * lsmblob_is_set - report if there is an value in the lsmblob
> + * @blob: Pointer to the exported LSM data
> + *
> + * Returns true if there is a secid set, false otherwise
> + */
> +static inline bool lsmblob_is_set(struct lsmblob *blob)
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		if (blob->secid[i] != 0)
> +			return true;
> +	return false;
> +}
> +
> +/**
> + * lsmblob_equal - report if the two lsmblob's are equal
> + * @bloba: Pointer to one LSM data
> + * @blobb: 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 *bloba, struct lsmblob *blobb)
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		if (bloba->secid[i] != blobb->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..7618c761060d 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,45 @@ 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 = LSMBLOB_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.getprocattr ||
> +		    hooks[i].head == &security_hook_heads.setprocattr ||
> +		    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 == LSMBLOB_INVALID) {
> +				slot = lsm_slot++;
> +				if (slot >= LSMBLOB_ENTRIES)
> +					panic("%s Too many LSMs registered.\n",
> +					      __func__);
> +				init_debug("%s assigned lsmblob slot %d\n",
> +					hooks[i].lsm, slot);
> +			}
> +		}
> +		hooks[i].slot = slot;

This is inconsistent, it sets hooks[i].slot before the first occurrence of a hook
in the above list to LSMBLOB_INVALID and the rest of the hooks[i].slots to the
assigned slot value.

If this should only be assigned to the hooks that use a secid then move the
hooks[i].slot = slot assignment into the if. Otherwise we should make sure all
hooks get assigned the same slot value.

>  	}
>  	if (lsm_append(lsm, &lsm_names) < 0)
>  		panic("%s - Cannot get early memory.\n", __func__);
> 