Re: [PATCH v30 03/28] LSM: Add the lsmblob data structure.

Paul Moore
2021-12-06T02:44:42Z

On Tue, Nov 23, 2021 at 8:47 PM Casey Schaufler <casey@schaufler-ca.com> 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.
> A new structure audit_rules is defined to avoid the
> confusion which commonly accompanies the use of
> void ** parameters.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/audit.h | 10 ++++-
> include/linux/lsm_hooks.h | 12 +++++-
> include/linux/security.h | 74 +++++++++++++++++++++++++++++---
> kernel/auditfilter.c | 24 ++++++-----
> kernel/auditsc.c | 17 +++-----
> security/apparmor/lsm.c | 7 ++-
> security/bpf/hooks.c | 12 +++++-
> security/commoncap.c | 7 ++-
> security/landlock/cred.c | 2 +-
> security/landlock/fs.c | 2 +-
> security/landlock/ptrace.c | 2 +-
> security/landlock/setup.c | 5 +++
> security/landlock/setup.h | 1 +
> security/loadpin/loadpin.c | 8 +++-
> security/lockdown/lockdown.c | 7 ++-
> security/safesetid/lsm.c | 8 +++-
> security/security.c | 82 ++++++++++++++++++++++++++++++------
> security/selinux/hooks.c | 8 +++-
> security/smack/smack_lsm.c | 7 ++-
> security/tomoyo/tomoyo.c | 8 +++-
> security/yama/yama_lsm.c | 7 ++-
> 21 files changed, 254 insertions(+), 56 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index d06134ac6245..943584128399 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -11,6 +11,7 @@
>
> #include <linux/sched.h>
> #include <linux/ptrace.h>
> +#include <linux/security.h>
> #include <linux/audit_arch.h>
> #include <uapi/linux/audit.h>
> #include <uapi/linux/netfilter/nf_tables.h>
> @@ -59,6 +60,10 @@ struct audit_krule {
> /* Flag to indicate legacy AUDIT_LOGINUID unset usage */
> #define AUDIT_LOGINUID_LEGACY 0x1
>
> +struct audit_rules {
> + void *rule[LSMBLOB_ENTRIES];
> +};

It would be nice to call this struct "audit_lsm_rules" to help
distinguish it from general audit filter rules.

> struct audit_field {
> u32 type;
> union {
> @@ -66,8 +71,9 @@ struct audit_field {
> kuid_t uid;
> kgid_t gid;
> struct {
> - char *lsm_str;
> - void *lsm_rule;
> + bool lsm_isset;
> + char *lsm_str;
> + struct audit_rules lsm_rules;
> };

Is lsm_isset strictly necessary? Unless I missed something it seems
like a NULL check on lsm_str would serve the same purpose.

--
paul moore