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

[Mimi Zohar]

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.

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.)

- 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.)

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

thanks,

Mimi

> 
> Acked-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> Acked-by: John Johansen <john.johansen@xxxxxxxxxxxxx>
> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

> Cc: <bpf@xxxxxxxxxxxxxxx>
> Cc: linux-audit@xxxxxxxxxx
> Cc: linux-security-module@xxxxxxxxxxxxxxx
> Cc: selinux@xxxxxxxxxxxxxxx
> ---

> 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