2023-06-29 19:59:49

by Casey Schaufler

[permalink] [raw]
Subject: [PATCH v12 02/11] LSM: Maintain a table of LSM attribute data

As LSMs are registered add their lsm_id pointers to a table.
This will be used later for attribute reporting.

Determine the number of possible security modules based on
their respective CONFIG options. This allows the number to be
known at build time. This allows data structures and tables
to use the constant.

Signed-off-by: Casey Schaufler <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Serge Hallyn <[email protected]>
---
include/linux/security.h | 2 ++
security/security.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index e2734e9e44d5..569b1d8ab002 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -138,6 +138,8 @@ enum lockdown_reason {
};

extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
+extern u32 lsm_active_cnt;
+extern struct lsm_id *lsm_idlist[];

/* These functions are in security/commoncap.c */
extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
diff --git a/security/security.c b/security/security.c
index e56714ef045a..5a699e47478b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -36,6 +36,25 @@
/* How many LSMs were built into the kernel? */
#define LSM_COUNT (__end_lsm_info - __start_lsm_info)

+/*
+ * How many LSMs are built into the kernel as determined at
+ * build time. Used to determine fixed array sizes.
+ * The capability module is accounted for by CONFIG_SECURITY
+ */
+#define LSM_CONFIG_COUNT ( \
+ (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_YAMA) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LOADPIN) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SAFESETID) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
+
/*
* These are descriptions of the reasons that can be passed to the
* security_locked_down() LSM hook. Placing this array here allows
@@ -245,6 +264,12 @@ static void __init initialize_lsm(struct lsm_info *lsm)
}
}

+/*
+ * Current index to use while initializing the lsm id list.
+ */
+u32 lsm_active_cnt __ro_after_init;
+struct lsm_id *lsm_idlist[LSM_CONFIG_COUNT] __ro_after_init;
+
/* Populate ordered LSMs list from comma-separated LSM name list. */
static void __init ordered_lsm_parse(const char *order, const char *origin)
{
@@ -521,6 +546,18 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
{
int i;

+ /*
+ * A security module may call security_add_hooks() more
+ * than once during initialization, and LSM initialization
+ * is serialized. Landlock is one such case.
+ * Look at the previous entry, if there is one, for duplication.
+ */
+ if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] != lsmid) {
+ if (lsm_active_cnt >= LSM_CONFIG_COUNT)
+ panic("%s Too many LSMs registered.\n", __func__);
+ lsm_idlist[lsm_active_cnt++] = lsmid;
+ }
+
for (i = 0; i < count; i++) {
hooks[i].lsmid = lsmid;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
--
2.40.1



2023-07-11 16:12:03

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH v12 02/11] LSM: Maintain a table of LSM attribute data


On 29/06/2023 21:55, Casey Schaufler wrote:
> As LSMs are registered add their lsm_id pointers to a table.
> This will be used later for attribute reporting.
>
> Determine the number of possible security modules based on
> their respective CONFIG options. This allows the number to be
> known at build time. This allows data structures and tables
> to use the constant.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Reviewed-by: Serge Hallyn <[email protected]>
> ---
> include/linux/security.h | 2 ++
> security/security.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e2734e9e44d5..569b1d8ab002 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -138,6 +138,8 @@ enum lockdown_reason {
> };
>
> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> +extern u32 lsm_active_cnt;
> +extern struct lsm_id *lsm_idlist[];

extern const struct lsm_id *lsm_idlist[];

>
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> diff --git a/security/security.c b/security/security.c
> index e56714ef045a..5a699e47478b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -36,6 +36,25 @@
> /* How many LSMs were built into the kernel? */
> #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
> +/*
> + * How many LSMs are built into the kernel as determined at
> + * build time. Used to determine fixed array sizes.
> + * The capability module is accounted for by CONFIG_SECURITY
> + */
> +#define LSM_CONFIG_COUNT ( \
> + (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_YAMA) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_LOADPIN) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SAFESETID) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
> +
> /*
> * These are descriptions of the reasons that can be passed to the
> * security_locked_down() LSM hook. Placing this array here allows
> @@ -245,6 +264,12 @@ static void __init initialize_lsm(struct lsm_info *lsm)
> }
> }
>
> +/*
> + * Current index to use while initializing the lsm id list.
> + */
> +u32 lsm_active_cnt __ro_after_init;
> +struct lsm_id *lsm_idlist[LSM_CONFIG_COUNT] __ro_after_init;

const struct lsm_id *lsm_idlist[LSM_CONFIG_COUNT] __ro_after_init;


> +
> /* Populate ordered LSMs list from comma-separated LSM name list. */
> static void __init ordered_lsm_parse(const char *order, const char *origin)
> {
> @@ -521,6 +546,18 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> {
> int i;
>
> + /*
> + * A security module may call security_add_hooks() more
> + * than once during initialization, and LSM initialization
> + * is serialized. Landlock is one such case.
> + * Look at the previous entry, if there is one, for duplication.
> + */
> + if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] != lsmid) {

Isn't it possible to have interleaved security_add_hooks() calls?


> + if (lsm_active_cnt >= LSM_CONFIG_COUNT)
> + panic("%s Too many LSMs registered.\n", __func__);

I'm not sure we should panic, but from a security point of view it is
critical enough…


> + lsm_idlist[lsm_active_cnt++] = lsmid;
> + }
> +
> for (i = 0; i < count; i++) {
> hooks[i].lsmid = lsmid;
> hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);

2023-07-14 20:36:26

by Casey Schaufler

[permalink] [raw]
Subject: Re: [PATCH v12 02/11] LSM: Maintain a table of LSM attribute data

On 7/11/2023 8:35 AM, Mickaël Salaün wrote:
>
> On 29/06/2023 21:55, Casey Schaufler wrote:
>> As LSMs are registered add their lsm_id pointers to a table.
>> This will be used later for attribute reporting.
>>
>> Determine the number of possible security modules based on
>> their respective CONFIG options. This allows the number to be
>> known at build time. This allows data structures and tables
>> to use the constant.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> Reviewed-by: Kees Cook <[email protected]>
>> Reviewed-by: Serge Hallyn <[email protected]>
>> ---
>>   include/linux/security.h |  2 ++
>>   security/security.c      | 37 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index e2734e9e44d5..569b1d8ab002 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -138,6 +138,8 @@ enum lockdown_reason {
>>   };
>>     extern const char *const
>> lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>> +extern u32 lsm_active_cnt;
>> +extern struct lsm_id *lsm_idlist[];
>
> extern const struct lsm_id *lsm_idlist[];
>
>>     /* These functions are in security/commoncap.c */
>>   extern int cap_capable(const struct cred *cred, struct
>> user_namespace *ns,
>> diff --git a/security/security.c b/security/security.c
>> index e56714ef045a..5a699e47478b 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -36,6 +36,25 @@
>>   /* How many LSMs were built into the kernel? */
>>   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>>   +/*
>> + * How many LSMs are built into the kernel as determined at
>> + * build time. Used to determine fixed array sizes.
>> + * The capability module is accounted for by CONFIG_SECURITY
>> + */
>> +#define LSM_CONFIG_COUNT ( \
>> +    (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_YAMA) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_LOADPIN) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_SAFESETID) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
>> +    (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
>> +
>>   /*
>>    * These are descriptions of the reasons that can be passed to the
>>    * security_locked_down() LSM hook. Placing this array here allows
>> @@ -245,6 +264,12 @@ static void __init initialize_lsm(struct
>> lsm_info *lsm)
>>       }
>>   }
>>   +/*
>> + * Current index to use while initializing the lsm id list.
>> + */
>> +u32 lsm_active_cnt __ro_after_init;
>> +struct lsm_id *lsm_idlist[LSM_CONFIG_COUNT] __ro_after_init;
>
> const struct lsm_id *lsm_idlist[LSM_CONFIG_COUNT] __ro_after_init;
>
>
>> +
>>   /* Populate ordered LSMs list from comma-separated LSM name list. */
>>   static void __init ordered_lsm_parse(const char *order, const char
>> *origin)
>>   {
>> @@ -521,6 +546,18 @@ void __init security_add_hooks(struct
>> security_hook_list *hooks, int count,
>>   {
>>       int i;
>>   +    /*
>> +     * A security module may call security_add_hooks() more
>> +     * than once during initialization, and LSM initialization
>> +     * is serialized. Landlock is one such case.
>> +     * Look at the previous entry, if there is one, for duplication.
>> +     */
>> +    if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] !=
>> lsmid) {
>
> Isn't it possible to have interleaved security_add_hooks() calls?

The initialization is serial and interleaving isn't possible.

>
>
>> +        if (lsm_active_cnt >= LSM_CONFIG_COUNT)
>> +            panic("%s Too many LSMs registered.\n", __func__);
>
> I'm not sure we should panic, but from a security point of view it is
> critical enough…

It's possible this should be a BUG() instance, but the panic() more
closely resembles what's nearby in the code.

>
>
>> +        lsm_idlist[lsm_active_cnt++] = lsmid;
>> +    }
>> +
>>       for (i = 0; i < count; i++) {
>>           hooks[i].lsmid = lsmid;
>>           hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);

2023-07-21 21:49:29

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v12 02/11] LSM: Maintain a table of LSM attribute data

On Fri, Jul 14, 2023 at 3:42 PM Casey Schaufler <[email protected]> wrote:
> On 7/11/2023 8:35 AM, Mickaël Salaün wrote:
> > On 29/06/2023 21:55, Casey Schaufler wrote:
> >> As LSMs are registered add their lsm_id pointers to a table.
> >> This will be used later for attribute reporting.
> >>
> >> Determine the number of possible security modules based on
> >> their respective CONFIG options. This allows the number to be
> >> known at build time. This allows data structures and tables
> >> to use the constant.
> >>
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> Reviewed-by: Kees Cook <[email protected]>
> >> Reviewed-by: Serge Hallyn <[email protected]>
> >> ---
> >> include/linux/security.h | 2 ++
> >> security/security.c | 37 +++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 39 insertions(+)

...

> >> diff --git a/security/security.c b/security/security.c
> >> index e56714ef045a..5a699e47478b 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -521,6 +546,18 @@ void __init security_add_hooks(struct
> >> security_hook_list *hooks, int count,
> >> {
> >> int i;
> >> + /*
> >> + * A security module may call security_add_hooks() more
> >> + * than once during initialization, and LSM initialization
> >> + * is serialized. Landlock is one such case.
> >> + * Look at the previous entry, if there is one, for duplication.
> >> + */
> >> + if (lsm_active_cnt == 0 || lsm_idlist[lsm_active_cnt - 1] !=
> >> lsmid) {
> >
> > Isn't it possible to have interleaved security_add_hooks() calls?
>
> The initialization is serial and interleaving isn't possible.
>
> >> + if (lsm_active_cnt >= LSM_CONFIG_COUNT)
> >> + panic("%s Too many LSMs registered.\n", __func__);
> >
> > I'm not sure we should panic, but from a security point of view it is
> > critical enough…
>
> It's possible this should be a BUG() instance, but the panic() more
> closely resembles what's nearby in the code.

I think the panic() call is okay. If something is so horribly broken
that we hit this case we have little option but to panic the system as
booting with the LSM controls busted in such a way is very not good.

There are probably those that would object to the above statement, but
those people aren't likely to be building a kernel with any LSMs in
the first place.

--
paul-moore.com