1:22 p.m.
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(a)tycho.nsa.gov>
> Acked-by: Paul Moore <paul(a)paul-moore.com>
> Acked-by: John Johansen <john.johansen(a)canonical.com>
> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com>
> Cc: <bpf(a)vger.kernel.org>
> Cc: linux-audit(a)redhat.com
> Cc: linux-security-module(a)vger.kernel.org
> Cc: selinux(a)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