diff mbox series

[v1,4/8] LSM: Maintain a table of LSM attribute data

Message ID 20221025184519.13231-5-casey@schaufler-ca.com
State New
Headers
Series LSM: Two basic syscalls |

Commit Message

Casey Schaufler Oct. 25, 2022, 6:45 p.m. UTC 
  As LSMs are registered add their lsm_id pointers to a table.
This will be used later for attribute reporting.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/security.h | 17 +++++++++++++++++
 security/security.c      | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Greg KH Oct. 26, 2022, 6 a.m. UTC | #1 
On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
> As LSMs are registered add their lsm_id pointers to a table.
> This will be used later for attribute reporting.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/security.h | 17 +++++++++++++++++
>  security/security.c      | 18 ++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0db..e1678594d983 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -138,6 +138,23 @@ enum lockdown_reason {
>  
>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>  
> +#define LSMID_ENTRIES ( \
> +	1 + /* capabilities */ \

No #define for capabilities?

> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
> +	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
> +
> +extern int lsm_id;

u64?

thanks,

greg k-h
Casey Schaufler Oct. 27, 2022, 12:38 a.m. UTC | #2 
On 10/25/2022 11:00 PM, Greg KH wrote:
> On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
>> As LSMs are registered add their lsm_id pointers to a table.
>> This will be used later for attribute reporting.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  include/linux/security.h | 17 +++++++++++++++++
>>  security/security.c      | 18 ++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index ca1b7109c0db..e1678594d983 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -138,6 +138,23 @@ enum lockdown_reason {
>>  
>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>>  
>> +#define LSMID_ENTRIES ( \
>> +	1 + /* capabilities */ \
> No #define for capabilities?

Nope. There isn't one. CONFIG_SECURITY takes care of it.

>> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
>> +	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
>> +
>> +extern int lsm_id;
> u64?

u32. I doubt we'll get more than 32K security modules.

>
> thanks,
>
> greg k-h
Greg KH Oct. 27, 2022, 6:29 a.m. UTC | #3 
On Wed, Oct 26, 2022 at 05:38:21PM -0700, Casey Schaufler wrote:
> On 10/25/2022 11:00 PM, Greg KH wrote:
> > On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
> >> As LSMs are registered add their lsm_id pointers to a table.
> >> This will be used later for attribute reporting.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >>  include/linux/security.h | 17 +++++++++++++++++
> >>  security/security.c      | 18 ++++++++++++++++++
> >>  2 files changed, 35 insertions(+)
> >>
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index ca1b7109c0db..e1678594d983 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -138,6 +138,23 @@ enum lockdown_reason {
> >>  
> >>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> >>  
> >> +#define LSMID_ENTRIES ( \
> >> +	1 + /* capabilities */ \
> > No #define for capabilities?
> 
> Nope. There isn't one. CONFIG_SECURITY takes care of it.
> 
> >> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> >> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> >> +	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
> >> +	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
> >> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
> >> +	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
> >> +
> >> +extern int lsm_id;
> > u64?
> 
> u32. I doubt we'll get more than 32K security modules.

These should be bits, not values, right?

Wait, this magic entry value is going to change depeneding on what is,
or is not, enabled.  How is that a stable user/kernel api at all?

confused.

greg k-h
Casey Schaufler Oct. 27, 2022, 5:08 p.m. UTC | #4 
On 10/26/2022 11:29 PM, Greg KH wrote:
> On Wed, Oct 26, 2022 at 05:38:21PM -0700, Casey Schaufler wrote:
>> On 10/25/2022 11:00 PM, Greg KH wrote:
>>> On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
>>>> As LSMs are registered add their lsm_id pointers to a table.
>>>> This will be used later for attribute reporting.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>>  include/linux/security.h | 17 +++++++++++++++++
>>>>  security/security.c      | 18 ++++++++++++++++++
>>>>  2 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>> index ca1b7109c0db..e1678594d983 100644
>>>> --- a/include/linux/security.h
>>>> +++ b/include/linux/security.h
>>>> @@ -138,6 +138,23 @@ enum lockdown_reason {
>>>>  
>>>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>>>>  
>>>> +#define LSMID_ENTRIES ( \
>>>> +	1 + /* capabilities */ \
>>> No #define for capabilities?
>> Nope. There isn't one. CONFIG_SECURITY takes care of it.
>>
>>>> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>>>> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>>>> +	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
>>>> +	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
>>>> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
>>>> +	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
>>>> +
>>>> +extern int lsm_id;
>>> u64?
>> u32. I doubt we'll get more than 32K security modules.
> These should be bits, not values, right?

lsm_id is the count of security modules that are registered.
It seemed like a good name for the value at the time, but as
it's causing confusion I should probably change it.

> Wait, this magic entry value is going to change depeneding on what is,
> or is not, enabled.  How is that a stable user/kernel api at all?
>
> confused.

I'll clarify.

This patch isn't implementing an API, but is required by subsequent
patches that do. Does linux-api want to see patches that are in support
of APIs, or just those with actual API implementation?

Thank you.

> greg k-h
Greg KH Oct. 27, 2022, 5:13 p.m. UTC | #5 
On Thu, Oct 27, 2022 at 10:08:23AM -0700, Casey Schaufler wrote:
> On 10/26/2022 11:29 PM, Greg KH wrote:
> > On Wed, Oct 26, 2022 at 05:38:21PM -0700, Casey Schaufler wrote:
> >> On 10/25/2022 11:00 PM, Greg KH wrote:
> >>> On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
> >>>> As LSMs are registered add their lsm_id pointers to a table.
> >>>> This will be used later for attribute reporting.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>> ---
> >>>>  include/linux/security.h | 17 +++++++++++++++++
> >>>>  security/security.c      | 18 ++++++++++++++++++
> >>>>  2 files changed, 35 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>>> index ca1b7109c0db..e1678594d983 100644
> >>>> --- a/include/linux/security.h
> >>>> +++ b/include/linux/security.h
> >>>> @@ -138,6 +138,23 @@ enum lockdown_reason {
> >>>>  
> >>>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> >>>>  
> >>>> +#define LSMID_ENTRIES ( \
> >>>> +	1 + /* capabilities */ \
> >>> No #define for capabilities?
> >> Nope. There isn't one. CONFIG_SECURITY takes care of it.
> >>
> >>>> +	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> >>>> +	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> >>>> +	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
> >>>> +	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
> >>>> +	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
> >>>> +	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
> >>>> +
> >>>> +extern int lsm_id;
> >>> u64?
> >> u32. I doubt we'll get more than 32K security modules.
> > These should be bits, not values, right?
> 
> lsm_id is the count of security modules that are registered.
> It seemed like a good name for the value at the time, but as
> it's causing confusion I should probably change it.

Yeah, that's confusing.  "lsm_num_availble" might be better.

> > Wait, this magic entry value is going to change depeneding on what is,
> > or is not, enabled.  How is that a stable user/kernel api at all?
> >
> > confused.
> 
> I'll clarify.
> 
> This patch isn't implementing an API, but is required by subsequent
> patches that do. Does linux-api want to see patches that are in support
> of APIs, or just those with actual API implementation?

There's nothing wrong with seeing this patch, I was just confused as it
seemed to be a user facing api.  It wasn't obvious to me, sorry.

greg k-h
Paul Moore Nov. 9, 2022, 11:34 p.m. UTC | #6 
On Wed, Oct 26, 2022 at 8:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/25/2022 11:00 PM, Greg KH wrote:
> > On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
> >> As LSMs are registered add their lsm_id pointers to a table.
> >> This will be used later for attribute reporting.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >>  include/linux/security.h | 17 +++++++++++++++++
> >>  security/security.c      | 18 ++++++++++++++++++
> >>  2 files changed, 35 insertions(+)
> >>
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index ca1b7109c0db..e1678594d983 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -138,6 +138,23 @@ enum lockdown_reason {
> >>
> >>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> >>
> >> +#define LSMID_ENTRIES ( \
> >> +    1 + /* capabilities */ \
> > No #define for capabilities?
>
> Nope. There isn't one. CONFIG_SECURITY takes care of it.

I guess we might as well use the existing pattern just in case this
header ever gets pulled into somewhere unexpected.

  (IS_ENABLED(CONFIG_SECURITY) ? 1 : 0) + ...

--
paul-moore.com
Paul Moore Nov. 9, 2022, 11:34 p.m. UTC | #7 
On Thu, Oct 27, 2022 at 1:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 27, 2022 at 10:08:23AM -0700, Casey Schaufler wrote:
> > On 10/26/2022 11:29 PM, Greg KH wrote:
> > > On Wed, Oct 26, 2022 at 05:38:21PM -0700, Casey Schaufler wrote:
> > >> On 10/25/2022 11:00 PM, Greg KH wrote:
> > >>> On Tue, Oct 25, 2022 at 11:45:15AM -0700, Casey Schaufler wrote:
> > >>>> As LSMs are registered add their lsm_id pointers to a table.
> > >>>> This will be used later for attribute reporting.
> > >>>>
> > >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > >>>> ---
> > >>>>  include/linux/security.h | 17 +++++++++++++++++
> > >>>>  security/security.c      | 18 ++++++++++++++++++
> > >>>>  2 files changed, 35 insertions(+)
> > >>>>
> > >>>> diff --git a/include/linux/security.h b/include/linux/security.h
> > >>>> index ca1b7109c0db..e1678594d983 100644
> > >>>> --- a/include/linux/security.h
> > >>>> +++ b/include/linux/security.h
> > >>>> @@ -138,6 +138,23 @@ enum lockdown_reason {
> > >>>>
> > >>>>  extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
> > >>>>
> > >>>> +#define LSMID_ENTRIES ( \
> > >>>> +        1 + /* capabilities */ \
> > >>> No #define for capabilities?
> > >> Nope. There isn't one. CONFIG_SECURITY takes care of it.
> > >>
> > >>>> +        (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> > >>>> +        (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> > >>>> +        (IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
> > >>>> +        (IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
> > >>>> +        (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
> > >>>> +        (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
> > >>>> +
> > >>>> +extern int lsm_id;
> > >>> u64?
> > >> u32. I doubt we'll get more than 32K security modules.
> > > These should be bits, not values, right?
> >
> > lsm_id is the count of security modules that are registered.
> > It seemed like a good name for the value at the time, but as
> > it's causing confusion I should probably change it.
>
> Yeah, that's confusing.  "lsm_num_availble" might be better.

Yes, this really should be named something else.  I'm partial to
"lsm_count" as it is shorter than the other suggestion, but this is
hardly something to worry too much about.

--
paul-moore.com
diff mbox series

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index ca1b7109c0db..e1678594d983 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -138,6 +138,23 @@  enum lockdown_reason {
 
 extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
 
+#define LSMID_ENTRIES ( \
+	1 + /* capabilities */ \
+	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_TOMOYO) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_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) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0))
+
+extern int lsm_id;
+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,
 		       int cap, unsigned int opts);
diff --git a/security/security.c b/security/security.c
index b2eb0ccd954b..bf206996a2af 100644
--- a/security/security.c
+++ b/security/security.c
@@ -28,6 +28,7 @@ 
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <linux/msg.h>
+#include <uapi/linux/lsm.h>
 #include <net/flow.h>
 
 #define MAX_LSM_EVM_XATTR	2
@@ -320,6 +321,12 @@  static void __init lsm_early_task(struct task_struct *task);
 
 static int lsm_append(const char *new, char **result);
 
+/*
+ * Current index to use while initializing the lsm id list.
+ */
+int lsm_id __lsm_ro_after_init;
+struct lsm_id *lsm_idlist[LSMID_ENTRIES] __lsm_ro_after_init;
+
 static void __init ordered_lsm_init(void)
 {
 	struct lsm_info **lsm;
@@ -364,6 +371,7 @@  static void __init ordered_lsm_init(void)
 	for (lsm = ordered_lsms; *lsm; lsm++)
 		initialize_lsm(*lsm);
 
+	init_debug("lsm count            = %d\n", lsm_id);
 	kfree(ordered_lsms);
 }
 
@@ -485,6 +493,16 @@  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. Landlock is one such case.
+	 */
+	if (lsm_id == 0 || lsm_idlist[lsm_id - 1] != lsmid)
+		lsm_idlist[lsm_id++] = lsmid;
+
+	if (lsm_id > LSMID_ENTRIES)
+		panic("%s Too many LSMs registered.\n", __func__);
+
 	for (i = 0; i < count; i++) {
 		hooks[i].lsmid = lsmid;
 		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);