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@xxxxxxxxxxxxxxxx>
>> ---
>> 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
>>
