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

[Casey Schaufler]

On 6/18/2019 9:52 PM, 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 at 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, is right. If the "char *lsm" pointer were replaced with a
"struct lsm_info *lsm_info" pointer, and "int slot" added to
lsm_info it would be a touch cleaner. A little bit of gimmickry
might be required for initialization, but maybe not. It's
worth looking into.

>>  
>>  /*
>> 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?

That would be about 7% better.

>> +	(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.

I figured this was either clever or hideous, but
hadn't fully decided which.

>> +
>> +#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*

I suppose, although you're adding compile time, and the
definition of LSMDATA_ENTRIES is just above.

>> +}
>> +
>> +/**
>> + * 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.

Point, and easy enough.

>> +				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
>>