Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

From: Casey Schaufler <casey@schaufler-ca.com>
To: Mimi Zohar <zohar@linux.ibm.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: linux-audit@redhat.com, keescook@chromium.org,
	john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
	paul@paul-moore.com, sds@tycho.nsa.gov,
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
Date: Mon, 28 Dec 2020 11:22:09 -0800	[thread overview]
Message-ID: <c88bc01f-3b65-f320-b42b-5ecde3e29448@schaufler-ca.com> (raw)
In-Reply-To: <903c37e9036d167958165ab700e646c1622a9c40.camel@linux.ibm.com>

On 12/28/2020 9:54 AM, Mimi Zohar wrote:
> Hi Casey,
>
> On Fri, 2020-11-20 at 12:14 -0800, 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.
>>
>> A new lsm_id structure, which contains the name
>> of the LSM and its slot number, is created. There
>> is an instance for each LSM, which assigns the name
>> and passes it to the infrastructure to set the slot.
>>
>> The audit rules data is expanded to use an array of
>> security module data rather than a single instance.
>> Because IMA uses the audit rule functions it is
>> affected as well.
> This patch is quite large, even without the audit rule change.  I would
> limit this patch to the new lsm_id structure changes.  The audit rule
> change should be broken out as a separate patch so that the audit
> changes aren't hidden.

Breaking up the patch in any meaningful way would require
scaffolding code that is as extensive and invasive as the
final change. I can do that if you really need it, but it
won't be any easier to read.

> In addition, here are a few high level nits:
> - The (patch description) body of the explanation, line wrapped at 75
> columns, which will be copied to the permanent changelog to describe
> this patch. (Refer  Documentation/process/submitting-patches.rst.)

Will fix.

> - The brief kernel-doc descriptions should not have a trailing period. 
> Nor should kernel-doc variable definitions have a trailing period. 
> Example(s) inline below.  (The existing kernel-doc is mostly correct.)

Will fix.

> - For some reason existing comments that span multiple lines aren't
> formatted properly.   In those cases, where there is another change,
> please fix the comment and function description.

Can you give an example? There are multiple comment styles
used in the various components.

> thanks,
>
> Mimi

I don't see any comments on the ima code changes. I really
don't want to spin a new patch set that does nothing but change
two periods in comments only to find out two months from now
that the code changes are completely borked. I really don't
want to go through the process of breaking up the patch that has
been widely Acked if there's no reason to expect it would require
significant work otherwise.

>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>> Acked-by: Paul Moore <paul@paul-moore.com>
>> Acked-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: <bpf@vger.kernel.org>
>> Cc: linux-audit@redhat.com
>> Cc: linux-security-module@vger.kernel.org
>> Cc: selinux@vger.kernel.org
>> ---
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index bc2725491560..fdb6e95c98e8 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -132,6 +132,65 @@ enum lockdown_reason {
>>
>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>>
>> +/*
>> + * Data exported by the security modules
>> + *
>> + * Any LSM that provides secid or secctx based hooks must be included.
>> + */
>> +#define LSMBLOB_ENTRIES ( \
>> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
>> +
>> +struct lsmblob {
>> +	u32     secid[LSMBLOB_ENTRIES];
>> +};
>> +
>> +#define LSMBLOB_INVALID		-1	/* Not a valid LSM slot number */
>> +#define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
>> +#define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
>> +
>> +/**
>> + * lsmblob_init - initialize an lsmblob structure.
> Only this kernel-doc brief description is suffixed with a period.  
> Please remove.
>
>> + * @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)
>> +{
>> +	struct lsmblob empty = {};
>> +
>> +	return !!memcmp(blob, &empty, sizeof(*blob));
>> +}
>> +
>> +/**
>> + * 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)
>> +{
>> +	return !memcmp(bloba, blobb, sizeof(*bloba));
>> +}
>> +
>>  /* These functions are in security/commoncap.c */
>>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 9b5adeaa47fc..cd393aaa17d5 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>>  	} lsm[MAX_LSM_RULES];
>> @@ -88,6 +88,22 @@ struct ima_rule_entry {
>>  	struct ima_template_desc *template;
>>  };
>>
>> +/**
>> + * ima_lsm_isset - Is a rule set for any of the active security modules
>> + * @rules: The set of IMA rules to check.
> Nor do kernel-doc variable definitions have a trailing period.
>
>> + *
>> + * If a rule is set for any LSM return true, otherwise return false.
>> + */
>> +static inline bool ima_lsm_isset(void *rules[])
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
>> +		if (rules[i])
>> +			return true;
>> +	return false;
>> +}
>> +
>>  /*
>>   * Without LSM specific knowledge, the default policy can only be
>>   * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner