[RFC PATCH -mm] LSM: Add lsm= boot parameter

[RFC PATCH -mm] LSM: Add lsm= boot parameter

[Ahmed S. Darwish]

Hi everybody,

This is a first try of adding lsm= boot parameter. 

Current situation is:
1- Ignore wrong input, with a small warning to users.
2- If user didn't specify a specific module, none will be loaded

Basically, the patch adds a @name attribute to each LSM. It
also adds a security_module_chosen(op) method where each
LSM _must_ pass before calling register_security().

Thanks,

 Documentation/kernel-parameters.txt |    4 ++++
 include/linux/security.h            |   10 ++++++++++
 security/dummy.c                    |    3 ++-
 security/security.c                 |   35 +++++++++++++++++++++++++++++++++++
 security/selinux/hooks.c            |    5 ++++-
 security/smack/smack_lsm.c          |    7 +++++++
 6 files changed, 62 insertions(+), 2 deletions(-)

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c64dfd7..dde04c8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -374,6 +374,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			possible to determine what the correct size should be.
 			This option provides an override for these situations.
 
+	lsm=		[SECURITY] Choose an LSM to enable at boot. If this boot
+			parameter is not specified, no security module will be 
+			loaded.
+
 	capability.disable=
 			[SECURITY] Disable capabilities.  This would normally
 			be used only if an alternative security model is to be
diff --git a/include/linux/security.h b/include/linux/security.h
index eb663e5..4f695c0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -42,6 +42,9 @@
 
 extern unsigned securebits;
 
+/* Maximum number of letters for an LSM name string */
+#define SECURITY_NAME_MAX	10
+
 struct ctl_table;
 struct audit_krule;
 
@@ -118,6 +121,10 @@ struct request_sock;
 /**
  * struct security_operations - main security structure
  *
+ * Security module identifier.
+ *
+ * @name: LSM name
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_alloc_security:
@@ -1262,6 +1269,8 @@ struct request_sock;
  * This is the main security structure.
  */
 struct security_operations {
+	char name[SECURITY_NAME_MAX + 1];
+
 	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
 	int (*capget) (struct task_struct * target,
 		       kernel_cap_t * effective,
@@ -1530,6 +1539,7 @@ struct security_operations {
 
 /* prototypes */
 extern int security_init	(void);
+extern int security_module_chosen(struct security_operations *ops);
 extern int register_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
diff --git a/security/dummy.c b/security/dummy.c
index 241ab20..ed11f97 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -1022,7 +1022,7 @@ static inline void dummy_audit_rule_free(void *lsmrule)
 
 #endif /* CONFIG_AUDIT */
 
-struct security_operations dummy_security_ops;
+struct security_operations dummy_security_ops = { "dummy" };
 
 #define set_to_dummy_if_null(ops, function)				\
 	do {								\
@@ -1035,6 +1035,7 @@ struct security_operations dummy_security_ops;
 
 void security_fixup_ops (struct security_operations *ops)
 {
+	BUG_ON(!ops->name);
 	set_to_dummy_if_null(ops, ptrace);
 	set_to_dummy_if_null(ops, capget);
 	set_to_dummy_if_null(ops, capset_check);
diff --git a/security/security.c b/security/security.c
index 1bf2ee4..7a84b4e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -17,6 +17,8 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 
+/* Boot time LSM user choice */
+char chosen_lsm[SECURITY_NAME_MAX + 1];
 
 /* things that live in dummy.c */
 extern struct security_operations dummy_security_ops;
@@ -67,6 +69,39 @@ int __init security_init(void)
 	return 0;
 }
 
+/* Save user chosen LSM */
+static int __init choose_lsm(char *str)
+{
+	if (strlen(str) > SECURITY_NAME_MAX) {
+		printk(KERN_INFO "Security: LSM name length extends possible "
+		       "limit.\n");
+		printk(KERN_INFO "Security: Ignoring passed lsm= parameter.\n");
+		return 0;
+	}
+
+	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	return 1;
+}
+__setup("lsm=", choose_lsm);
+
+/**
+ * security_module_chosen - Load given security module on boot ?
+ * @ops: a pointer to the struct security_operations that is to be checked.
+ *
+ * Return true if the passed LSM is the one chosen by user at
+ * boot time, otherwise return false.
+ */
+int security_module_chosen(struct security_operations *ops)
+{
+	if (!ops || !ops->name)
+		return 0;
+
+	if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
+		return 0;
+	
+	return 1;
+}
+
 /**
  * register_security - registers a security framework with the kernel
  * @ops: a pointer to the struct security_options that is to be registered
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bef1834..d4926b0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5247,6 +5247,8 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_operations selinux_ops = {
+	.name =				"selinux",
+
 	.ptrace =			selinux_ptrace,
 	.capget =			selinux_capget,
 	.capset_check =			selinux_capset_check,
@@ -5443,7 +5445,8 @@ static __init int selinux_init(void)
 {
 	struct task_security_struct *tsec;
 
-	if (!selinux_enabled) {
+	if (!selinux_enabled || !security_module_chosen(&selinux_ops)) {
+		selinux_enabled = 0;
 		printk(KERN_INFO "SELinux:  Disabled at boot.\n");
 		return 0;
 	}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b5d6f7..4348257 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 }
 
 static struct security_operations smack_ops = {
+	.name =				"smack",
+
 	.ptrace = 			smack_ptrace,
 	.capget = 			cap_capget,
 	.capset_check = 		cap_capset_check,
@@ -2485,6 +2487,11 @@ static struct security_operations smack_ops = {
  */
 static __init int smack_init(void)
 {
+	if (!security_module_chosen(&smack_ops)) {
+		printk(KERN_INFO "Smack: Disabled at boot.\n");
+		return 0;
+	}
+
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
 	/*

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [RFC PATCH -mm] LSM: Add lsm= boot parameter

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi everybody,
> 
> This is a first try of adding lsm= boot parameter. 
> 
> Current situation is:
> 1- Ignore wrong input, with a small warning to users.
> 2- If user didn't specify a specific module, none will be loaded

I'm not fond of this behavior for the case where only one LSM
has been built in. Fedora, for example, ought to boot SELinux
without specifing lsm=SELinux, and all the rest should boot
whatever they are built with. In the case where a kernel is
built with conflicting LSMs (today SELinux and Smack) I see
this as a useful way to decide which to use until you get
your kernel rebuilt sanely, so it appears to be worth having.

> 
> Basically, the patch adds a @name attribute to each LSM. It
> also adds a security_module_chosen(op) method where each
> LSM _must_ pass before calling register_security().
> 
> Thanks,
> 
>  Documentation/kernel-parameters.txt |    4 ++++
>  include/linux/security.h            |   10 ++++++++++
>  security/dummy.c                    |    3 ++-
>  security/security.c                 |   35
> +++++++++++++++++++++++++++++++++++
>  security/selinux/hooks.c            |    5 ++++-
>  security/smack/smack_lsm.c          |    7 +++++++
>  6 files changed, 62 insertions(+), 2 deletions(-)
> 
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> ---
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index c64dfd7..dde04c8 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -374,6 +374,10 @@ and is between 256 and 4096 characters. It is defined in
> the file
>  			possible to determine what the correct size should be.
>  			This option provides an override for these situations.
>  
> +	lsm=		[SECURITY] Choose an LSM to enable at boot. If this boot
> +			parameter is not specified, no security module will be 
> +			loaded.
> +
>  	capability.disable=
>  			[SECURITY] Disable capabilities.  This would normally
>  			be used only if an alternative security model is to be
> diff --git a/include/linux/security.h b/include/linux/security.h
> index eb663e5..4f695c0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -42,6 +42,9 @@
>  
>  extern unsigned securebits;
>  
> +/* Maximum number of letters for an LSM name string */
> +#define SECURITY_NAME_MAX	10
> +
>  struct ctl_table;
>  struct audit_krule;
>  
> @@ -118,6 +121,10 @@ struct request_sock;
>  /**
>   * struct security_operations - main security structure
>   *
> + * Security module identifier.
> + *
> + * @name: LSM name
> + *
>   * Security hooks for program execution operations.
>   *
>   * @bprm_alloc_security:
> @@ -1262,6 +1269,8 @@ struct request_sock;
>   * This is the main security structure.
>   */
>  struct security_operations {
> +	char name[SECURITY_NAME_MAX + 1];
> +
>  	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
>  	int (*capget) (struct task_struct * target,
>  		       kernel_cap_t * effective,
> @@ -1530,6 +1539,7 @@ struct security_operations {
>  
>  /* prototypes */
>  extern int security_init	(void);
> +extern int security_module_chosen(struct security_operations *ops);
>  extern int register_security	(struct security_operations *ops);
>  extern int mod_reg_security	(const char *name, struct security_operations
> *ops);
>  extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
> diff --git a/security/dummy.c b/security/dummy.c
> index 241ab20..ed11f97 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -1022,7 +1022,7 @@ static inline void dummy_audit_rule_free(void *lsmrule)
>  
>  #endif /* CONFIG_AUDIT */
>  
> -struct security_operations dummy_security_ops;
> +struct security_operations dummy_security_ops = { "dummy" };
>  
>  #define set_to_dummy_if_null(ops, function)				\
>  	do {								\
> @@ -1035,6 +1035,7 @@ struct security_operations dummy_security_ops;
>  
>  void security_fixup_ops (struct security_operations *ops)
>  {
> +	BUG_ON(!ops->name);
>  	set_to_dummy_if_null(ops, ptrace);
>  	set_to_dummy_if_null(ops, capget);
>  	set_to_dummy_if_null(ops, capset_check);
> diff --git a/security/security.c b/security/security.c
> index 1bf2ee4..7a84b4e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -17,6 +17,8 @@
>  #include <linux/kernel.h>
>  #include <linux/security.h>
>  
> +/* Boot time LSM user choice */
> +char chosen_lsm[SECURITY_NAME_MAX + 1];
>  
>  /* things that live in dummy.c */
>  extern struct security_operations dummy_security_ops;
> @@ -67,6 +69,39 @@ int __init security_init(void)
>  	return 0;
>  }
>  
> +/* Save user chosen LSM */
> +static int __init choose_lsm(char *str)
> +{
> +	if (strlen(str) > SECURITY_NAME_MAX) {
> +		printk(KERN_INFO "Security: LSM name length extends possible "
> +		       "limit.\n");
> +		printk(KERN_INFO "Security: Ignoring passed lsm= parameter.\n");
> +		return 0;
> +	}
> +
> +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> +	return 1;
> +}
> +__setup("lsm=", choose_lsm);
> +
> +/**
> + * security_module_chosen - Load given security module on boot ?
> + * @ops: a pointer to the struct security_operations that is to be checked.
> + *
> + * Return true if the passed LSM is the one chosen by user at
> + * boot time, otherwise return false.
> + */
> +int security_module_chosen(struct security_operations *ops)
> +{
> +	if (!ops || !ops->name)
> +		return 0;
> +
> +	if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> +		return 0;
> +	
> +	return 1;
> +}
> +
>  /**
>   * register_security - registers a security framework with the kernel
>   * @ops: a pointer to the struct security_options that is to be registered
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index bef1834..d4926b0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5247,6 +5247,8 @@ static int selinux_key_getsecurity(struct key *key,
> char **_buffer)
>  #endif
>  
>  static struct security_operations selinux_ops = {
> +	.name =				"selinux",
> +
>  	.ptrace =			selinux_ptrace,
>  	.capget =			selinux_capget,
>  	.capset_check =			selinux_capset_check,
> @@ -5443,7 +5445,8 @@ static __init int selinux_init(void)
>  {
>  	struct task_security_struct *tsec;
>  
> -	if (!selinux_enabled) {
> +	if (!selinux_enabled || !security_module_chosen(&selinux_ops)) {
> +		selinux_enabled = 0;
>  		printk(KERN_INFO "SELinux:  Disabled at boot.\n");
>  		return 0;
>  	}
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2b5d6f7..4348257 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32
> seclen)
>  }
>  
>  static struct security_operations smack_ops = {
> +	.name =				"smack",
> +
>  	.ptrace = 			smack_ptrace,
>  	.capget = 			cap_capget,
>  	.capset_check = 		cap_capset_check,
> @@ -2485,6 +2487,11 @@ static struct security_operations smack_ops = {
>   */
>  static __init int smack_init(void)
>  {
> +	if (!security_module_chosen(&smack_ops)) {
> +		printk(KERN_INFO "Smack: Disabled at boot.\n");
> +		return 0;
> +	}
> +
>  	printk(KERN_INFO "Smack:  Initializing.\n");
>  
>  	/*
> 
> -- 
> 
> "Better to light a candle, than curse the darkness"
> 
> Ahmed S. Darwish
> Homepage: http://darwish.07.googlepages.com
> Blog: http://darwish-07.blogspot.com
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: [RFC PATCH -mm] LSM: Add lsm= boot parameter

[Adrian Bunk]

On Sat, Mar 01, 2008 at 12:28:43PM -0800, Casey Schaufler wrote:
> 
> --- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> 
> > Hi everybody,
> > 
> > This is a first try of adding lsm= boot parameter. 
> > 
> > Current situation is:
> > 1- Ignore wrong input, with a small warning to users.
> > 2- If user didn't specify a specific module, none will be loaded
> 
> I'm not fond of this behavior for the case where only one LSM
> has been built in. Fedora, for example, ought to boot SELinux
> without specifing lsm=SELinux, and all the rest should boot
> whatever they are built with. In the case where a kernel is
> built with conflicting LSMs (today SELinux and Smack) I see
> this as a useful way to decide which to use until you get
> your kernel rebuilt sanely, so it appears to be worth having.
>...

Remarks:

Your comment would be covered if the default for this boot parameter (if 
not explicitely set through the boot loader would not be "disabled" but 
set through kconfig (based on the selected LSMs).

We should really get this resolved for 2.6.25.

security= suggestion is IMHO more intuitive than lsm=

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC PATCH -mm] LSM: Add lsm= boot parameter

[Casey Schaufler]

--- Adrian Bunk <bunk@kernel.org> wrote:

> On Sat, Mar 01, 2008 at 12:28:43PM -0800, Casey Schaufler wrote:
> > 
> > --- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> > 
> > > Hi everybody,
> > > 
> > > This is a first try of adding lsm= boot parameter. 
> > > 
> > > Current situation is:
> > > 1- Ignore wrong input, with a small warning to users.
> > > 2- If user didn't specify a specific module, none will be loaded
> > 
> > I'm not fond of this behavior for the case where only one LSM
> > has been built in. Fedora, for example, ought to boot SELinux
> > without specifing lsm=SELinux, and all the rest should boot
> > whatever they are built with. In the case where a kernel is
> > built with conflicting LSMs (today SELinux and Smack) I see
> > this as a useful way to decide which to use until you get
> > your kernel rebuilt sanely, so it appears to be worth having.
> >...
> 
> Remarks:
> 
> Your comment would be covered if the default for this boot parameter (if 
> not explicitely set through the boot loader would not be "disabled" but 
> set through kconfig (based on the selected LSMs).

Agreed.

> We should really get this resolved for 2.6.25.

Agreed.

> security= suggestion is IMHO more intuitive than lsm=

security is a very overloaded term, but since this is one
of the ways it's already loaded in I could be OK with that.


Casey Schaufler
casey@schaufler-ca.com

[PATCH -v2 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

Hi!,

Add the security= boot parameter. This is done to avoid LSM 
registration clashes in case of more than one bult-in module.

User can choose a security module to enable at boot. If no 
security= boot parameter is specified, only the first LSM 
asking for registration will be loaded. An invalid security 
module name will be treated as if no module has been chosen.

LSM modules must check now if they are allowed to register
by calling security_module_enable(ops) first. Modify SELinux 
and SMACK to do so.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

I'll send a similar patch for 2.6.25 if no more concerns for
the patch exist. 

Adrian, Casey, Does this one have any more issues ?

 Documentation/kernel-parameters.txt |    6 ++++
 include/linux/security.h            |   12 +++++++++
 security/dummy.c                    |    3 +-
 security/security.c                 |   47 +++++++++++++++++++++++++++++++++++-
 security/selinux/hooks.c            |    5 +++
 security/smack/smack_lsm.c          |    7 +++++

 6 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c64dfd7..85044e8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -374,6 +374,12 @@ and is between 256 and 4096 characters. It is defined in the file
 			possible to determine what the correct size should be.
 			This option provides an override for these situations.
 
+	security=	[SECURITY] Choose a security module to enable at boot. 
+			If this boot parameter is not specified, only the first 
+			security module asking for security registration will be
+			loaded. An invalid security module name will be treated
+			as if no module has been chosen.
+
 	capability.disable=
 			[SECURITY] Disable capabilities.  This would normally
 			be used only if an alternative security model is to be
diff --git a/include/linux/security.h b/include/linux/security.h
index a33fd03..416afcf 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -42,6 +42,9 @@
 
 extern unsigned securebits;
 
+/* Maximum number of letters for an LSM name string */
+#define SECURITY_NAME_MAX	10
+
 struct ctl_table;
 
 /*
@@ -117,6 +120,12 @@ struct request_sock;
 /**
  * struct security_operations - main security structure
  *
+ * Security module identifier.
+ *
+ * @name:
+ *	A string that acts as  unique identifeir for the LSM with max number 
+ *	of characters = SECURITY_NAME_MAX.
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_alloc_security:
@@ -1218,6 +1227,8 @@ struct request_sock;
  * This is the main security structure.
  */
 struct security_operations {
+	char name[SECURITY_NAME_MAX + 1];
+
 	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
 	int (*capget) (struct task_struct * target,
 		       kernel_cap_t * effective,
@@ -1477,6 +1488,7 @@ struct security_operations {
 
 /* prototypes */
 extern int security_init	(void);
+extern int security_module_enable(struct security_operations *ops);
 extern int register_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
diff --git a/security/dummy.c b/security/dummy.c
index 6a0056b..7cb999c 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -986,7 +986,7 @@ static int dummy_key_getsecurity(struct key *key, char **_buffer)
 
 #endif /* CONFIG_KEYS */
 
-struct security_operations dummy_security_ops;
+struct security_operations dummy_security_ops = { "dummy" };
 
 #define set_to_dummy_if_null(ops, function)				\
 	do {								\
@@ -999,6 +999,7 @@ struct security_operations dummy_security_ops;
 
 void security_fixup_ops (struct security_operations *ops)
 {
+	BUG_ON(!ops->name);
 	set_to_dummy_if_null(ops, ptrace);
 	set_to_dummy_if_null(ops, capget);
 	set_to_dummy_if_null(ops, capset_check);
diff --git a/security/security.c b/security/security.c
index 3e75b90..261d2e4 100644
--- a/security/security.c
+++ b/security/security.c
@@ -17,6 +17,9 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 
+/* Boot-time LSM user choice */
+static char chosen_lsm[SECURITY_NAME_MAX + 1];
+static atomic_t security_ops_registered = ATOMIC_INIT(0);
 
 /* things that live in dummy.c */
 extern struct security_operations dummy_security_ops;
@@ -67,13 +70,54 @@ int __init security_init(void)
 	return 0;
 }
 
+/* Save user chosen LSM */
+static int __init choose_lsm(char *str)
+{
+	if (strlen(str) > SECURITY_NAME_MAX) {
+		printk(KERN_INFO "Security: LSM name length extends possible"
+		       "limit.\n");
+		printk(KERN_INFO "Security: Ignoring passed security= "
+		       "parameter.\n");
+		return 0;
+	}
+
+	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	return 1;
+}
+__setup("security=", choose_lsm);
+
+/**
+ * security_module_enable - Load given security module on boot ?
+ * @ops: a pointer to the struct security_operations that is to be checked.
+ *
+ * Return true if:
+ *	-The passed LSM is the one chosen by user at boot time,
+ *	-or user didsn't specify a specific LSM and we're the first to ask
+ *	 for registeration permissoin.
+ * Otherwise, return false.
+ */
+int security_module_enable(struct security_operations *ops)
+{
+	if (!ops || !ops->name)
+		return 0;
+
+	if (!*chosen_lsm && !atomic_read(&security_ops_registered))
+		return 1;
+
+	if (!strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
+		return 1;
+	
+	return 0;
+}
+
 /**
  * register_security - registers a security framework with the kernel
  * @ops: a pointer to the struct security_options that is to be registered
  *
  * This function is to allow a security module to register itself with the
  * kernel security subsystem.  Some rudimentary checking is done on the @ops
- * value passed to this function.
+ * value passed to this function. You'll need to check first if your LSM
+ * is allowed to register by calling security_module_enable(@ops).
  *
  * If there is already a security module registered with the kernel,
  * an error will be returned.  Otherwise 0 is returned on success.
@@ -90,6 +134,7 @@ int register_security(struct security_operations *ops)
 		return -EAGAIN;
 
 	security_ops = ops;
+	atomic_inc(&security_ops_registered);
 
 	return 0;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f42ebfc..fe30d2b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5233,6 +5233,8 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_operations selinux_ops = {
+	.name =				"selinux",
+
 	.ptrace =			selinux_ptrace,
 	.capget =			selinux_capget,
 	.capset_check =			selinux_capset_check,
@@ -5420,7 +5422,8 @@ static __init int selinux_init(void)
 {
 	struct task_security_struct *tsec;
 
-	if (!selinux_enabled) {
+	if (!selinux_enabled || !security_module_enable(&selinux_ops)) {
+		selinux_enabled = 0;
 		printk(KERN_INFO "SELinux:  Disabled at boot.\n");
 		return 0;
 	}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b5d6f7..3fc8c6e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 }
 
 static struct security_operations smack_ops = {
+	.name =				"smack",
+
 	.ptrace = 			smack_ptrace,
 	.capget = 			cap_capget,
 	.capset_check = 		cap_capset_check,
@@ -2485,6 +2487,11 @@ static struct security_operations smack_ops = {
  */
 static __init int smack_init(void)
 {
+	if (!security_module_enable(&smack_ops)) {
+		printk(KERN_INFO "Smack: Disabled at boot.\n");
+		return 0;
+	}
+
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
 	/*

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v2 -mm] LSM: Add security= boot parameter

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi!,
> 
> Add the security= boot parameter. This is done to avoid LSM 
> registration clashes in case of more than one bult-in module.
> 
> User can choose a security module to enable at boot. If no 
> security= boot parameter is specified, only the first LSM 
> asking for registration will be loaded. An invalid security 
> module name will be treated as if no module has been chosen.
> 
> LSM modules must check now if they are allowed to register
> by calling security_module_enable(ops) first. Modify SELinux 
> and SMACK to do so.
> 
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> ---
> 
> I'll send a similar patch for 2.6.25 if no more concerns for
> the patch exist. 
> 
> Adrian, Casey, Does this one have any more issues ?
> 
>  Documentation/kernel-parameters.txt |    6 ++++
>  include/linux/security.h            |   12 +++++++++
>  security/dummy.c                    |    3 +-
>  security/security.c                 |   47
> +++++++++++++++++++++++++++++++++++-
>  security/selinux/hooks.c            |    5 +++
>  security/smack/smack_lsm.c          |    7 +++++
> 
>  6 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index c64dfd7..85044e8 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -374,6 +374,12 @@ and is between 256 and 4096 characters. It is defined in
> the file
>  			possible to determine what the correct size should be.
>  			This option provides an override for these situations.
>  
> +	security=	[SECURITY] Choose a security module to enable at boot. 
> +			If this boot parameter is not specified, only the first 
> +			security module asking for security registration will be
> +			loaded. An invalid security module name will be treated
> +			as if no module has been chosen.
> +

Yes, this is better.

>  	capability.disable=
>  			[SECURITY] Disable capabilities.  This would normally
>  			be used only if an alternative security model is to be
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a33fd03..416afcf 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -42,6 +42,9 @@
>  
>  extern unsigned securebits;
>  
> +/* Maximum number of letters for an LSM name string */
> +#define SECURITY_NAME_MAX	10
> +
>  struct ctl_table;
>  
>  /*
> @@ -117,6 +120,12 @@ struct request_sock;
>  /**
>   * struct security_operations - main security structure
>   *
> + * Security module identifier.
> + *
> + * @name:
> + *	A string that acts as  unique identifeir for the LSM with max number 
> + *	of characters = SECURITY_NAME_MAX.
> + *
>   * Security hooks for program execution operations.
>   *
>   * @bprm_alloc_security:
> @@ -1218,6 +1227,8 @@ struct request_sock;
>   * This is the main security structure.
>   */
>  struct security_operations {
> +	char name[SECURITY_NAME_MAX + 1];
> +
>  	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
>  	int (*capget) (struct task_struct * target,
>  		       kernel_cap_t * effective,
> @@ -1477,6 +1488,7 @@ struct security_operations {
>  
>  /* prototypes */
>  extern int security_init	(void);
> +extern int security_module_enable(struct security_operations *ops);
>  extern int register_security	(struct security_operations *ops);
>  extern int mod_reg_security	(const char *name, struct security_operations
> *ops);
>  extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
> diff --git a/security/dummy.c b/security/dummy.c
> index 6a0056b..7cb999c 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -986,7 +986,7 @@ static int dummy_key_getsecurity(struct key *key, char
> **_buffer)
>  
>  #endif /* CONFIG_KEYS */
>  
> -struct security_operations dummy_security_ops;
> +struct security_operations dummy_security_ops = { "dummy" };
>  
>  #define set_to_dummy_if_null(ops, function)				\
>  	do {								\
> @@ -999,6 +999,7 @@ struct security_operations dummy_security_ops;
>  
>  void security_fixup_ops (struct security_operations *ops)
>  {
> +	BUG_ON(!ops->name);
>  	set_to_dummy_if_null(ops, ptrace);
>  	set_to_dummy_if_null(ops, capget);
>  	set_to_dummy_if_null(ops, capset_check);
> diff --git a/security/security.c b/security/security.c
> index 3e75b90..261d2e4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -17,6 +17,9 @@
>  #include <linux/kernel.h>
>  #include <linux/security.h>
>  
> +/* Boot-time LSM user choice */
> +static char chosen_lsm[SECURITY_NAME_MAX + 1];
> +static atomic_t security_ops_registered = ATOMIC_INIT(0);
>  
>  /* things that live in dummy.c */
>  extern struct security_operations dummy_security_ops;
> @@ -67,13 +70,54 @@ int __init security_init(void)
>  	return 0;
>  }
>  
> +/* Save user chosen LSM */
> +static int __init choose_lsm(char *str)
> +{
> +	if (strlen(str) > SECURITY_NAME_MAX) {
> +		printk(KERN_INFO "Security: LSM name length extends possible"
> +		       "limit.\n");
> +		printk(KERN_INFO "Security: Ignoring passed security= "
> +		       "parameter.\n");
> +		return 0;
> +	}
> +
> +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> +	return 1;
> +}
> +__setup("security=", choose_lsm);
> +
> +/**
> + * security_module_enable - Load given security module on boot ?
> + * @ops: a pointer to the struct security_operations that is to be checked.
> + *
> + * Return true if:
> + *	-The passed LSM is the one chosen by user at boot time,
> + *	-or user didsn't specify a specific LSM and we're the first to ask
> + *	 for registeration permissoin.
> + * Otherwise, return false.
> + */
> +int security_module_enable(struct security_operations *ops)
> +{
> +	if (!ops || !ops->name)
> +		return 0;
> +
> +	if (!*chosen_lsm && !atomic_read(&security_ops_registered))
> +		return 1;
> +
> +	if (!strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> +		return 1;
> +	
> +	return 0;
> +}
> +
>  /**
>   * register_security - registers a security framework with the kernel
>   * @ops: a pointer to the struct security_options that is to be registered
>   *
>   * This function is to allow a security module to register itself with the
>   * kernel security subsystem.  Some rudimentary checking is done on the @ops
> - * value passed to this function.
> + * value passed to this function. You'll need to check first if your LSM
> + * is allowed to register by calling security_module_enable(@ops).
>   *
>   * If there is already a security module registered with the kernel,
>   * an error will be returned.  Otherwise 0 is returned on success.
> @@ -90,6 +134,7 @@ int register_security(struct security_operations *ops)
>  		return -EAGAIN;
>  
>  	security_ops = ops;
> +	atomic_inc(&security_ops_registered);
>  
>  	return 0;
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f42ebfc..fe30d2b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5233,6 +5233,8 @@ static int selinux_key_getsecurity(struct key *key,
> char **_buffer)
>  #endif
>  
>  static struct security_operations selinux_ops = {
> +	.name =				"selinux",
> +
>  	.ptrace =			selinux_ptrace,
>  	.capget =			selinux_capget,
>  	.capset_check =			selinux_capset_check,
> @@ -5420,7 +5422,8 @@ static __init int selinux_init(void)
>  {
>  	struct task_security_struct *tsec;
>  
> -	if (!selinux_enabled) {
> +	if (!selinux_enabled || !security_module_enable(&selinux_ops)) {
> +		selinux_enabled = 0;
>  		printk(KERN_INFO "SELinux:  Disabled at boot.\n");

How about "SELinux: Not enabled because LSM %s is already enabled.\n"

>  		return 0;
>  	}
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2b5d6f7..3fc8c6e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32
> seclen)
>  }
>  
>  static struct security_operations smack_ops = {
> +	.name =				"smack",
> +
>  	.ptrace = 			smack_ptrace,
>  	.capget = 			cap_capget,
>  	.capset_check = 		cap_capset_check,
> @@ -2485,6 +2487,11 @@ static struct security_operations smack_ops = {
>   */
>  static __init int smack_init(void)
>  {
> +	if (!security_module_enable(&smack_ops)) {
> +		printk(KERN_INFO "Smack: Disabled at boot.\n");

How about "Smack: Not enabled because LSM %s is already enabled.\n"

> +		return 0;
> +	}
> +
>  	printk(KERN_INFO "Smack:  Initializing.\n");
>  
>  	/*
> 
> -- 
> 
> "Better to light a candle, than curse the darkness"
> 
> Ahmed S. Darwish
> Homepage: http://darwish.07.googlepages.com
> Blog: http://darwish-07.blogspot.com
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH -v2 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

On Sun, Mar 02, 2008 at 01:27:08AM +0200, Ahmed S. Darwish wrote:
> Hi!,
> 
... 
> LSM modules must check now if they are allowed to register
> by calling security_module_enable(ops) first. Modify SELinux 
> and SMACK to do so.
> 
...
>  
> +/* Boot-time LSM user choice */
> +static char chosen_lsm[SECURITY_NAME_MAX + 1];
> +static atomic_t security_ops_registered = ATOMIC_INIT(0);
>  
...
> +int security_module_enable(struct security_operations *ops)
> +{
> +	if (!ops || !ops->name)
> +		return 0;
> +
> +	if (!*chosen_lsm && !atomic_read(&security_ops_registered))
> +		return 1;
> +
...
> @@ -90,6 +134,7 @@ int register_security(struct security_operations *ops)
>  		return -EAGAIN;
>  
>  	security_ops = ops;
> +	atomic_inc(&security_ops_registered);
>  

I'm worried about an implementation detail here. Must the LSM
init calls sequence:
asmlinkage void __init start_kernel(void)
{
	preempt_disable();
	...
	security_init();
	...

int __init security_init(void)
{
	...
	do_security_initcalls();
}
static void __init do_security_initcalls(void)
{
	initcall_t *call;
	call = __security_initcall_start;
	while (call < __security_initcall_end) {
		(*call) ();
		call++;
	}
}
be SMP safe ?

In other words, can the two LSMs 'security_initcall()'s 
(i.e. smack_init() and selinux_init()) be executed concurrently ?

If so, this patch won't be safe.
I'll send a modified one once I know the answer.

Thanks everybody,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v2 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

On Sat, Mar 01, 2008 at 07:41:04PM -0800, Casey Schaufler wrote:
> 
> --- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> >
...
> > 
> >  
> >  static struct security_operations selinux_ops = {
> > +	.name =				"selinux",
> > +
> >  	.ptrace =			selinux_ptrace,
> >  	.capget =			selinux_capget,
> >  	.capset_check =			selinux_capset_check,
> > @@ -5420,7 +5422,8 @@ static __init int selinux_init(void)
> >  {
> >  	struct task_security_struct *tsec;
> >  
> > -	if (!selinux_enabled) {
> > +	if (!selinux_enabled || !security_module_enable(&selinux_ops)) {
> > +		selinux_enabled = 0;
> >  		printk(KERN_INFO "SELinux:  Disabled at boot.\n");
> 
> How about "SELinux: Not enabled because LSM %s is already enabled.\n"
> 

Looks better. I'll resend the patch once I know the answer of the SMP
point I asked about in the same thread.

Regards,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

[PATCH -v3 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

Hi!,

[
Fixed two bugs:
       - concurrency: incrementing and testing atomic_t in different places.
       - overflow: not ending string with NULL after using strncpy().
       - I'll never write a patch when I'm asleep, sorry :(

Added more verbose messages to SMACK and SELinux if they were not 
chosen on boot.

Casey: Failing to take permission to register an LSM does not mean that 
       the other has registered its security_ops yet. It just means that
       the other asked for allowance to call register_security(). It's 
       not yet guraranteed that this registration succeeded.

       This means that adding "SELinux: failed to load, LSM %s is loaded"
       may lead to %s = "dummy" in case of a highly concurrent SMP system.
]

-->

Add the security= boot parameter. This is done to avoid LSM 
registration clashes in case of more than one bult-in module.

User can choose a security module to enable at boot. If no 
security= boot parameter is specified, only the first LSM 
asking for registration will be loaded. An invalid security 
module name will be treated as if no module has been chosen.

LSM modules must check now if they are allowed to register
by calling security_module_enable(ops) first. Modify SELinux 
and SMACK to do so.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 Documentation/kernel-parameters.txt |    6 ++++
 include/linux/security.h            |   12 +++++++++
 security/dummy.c                    |    2 -
 security/security.c                 |   46 +++++++++++++++++++++++++++++++++---
 security/selinux/hooks.c            |   12 +++++++++
 security/smack/smack_lsm.c          |   11 ++++++++

 6 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c64dfd7..85044e8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -374,6 +374,12 @@ and is between 256 and 4096 characters. It is defined in the file
 			possible to determine what the correct size should be.
 			This option provides an override for these situations.
 
+	security=	[SECURITY] Choose a security module to enable at boot. 
+			If this boot parameter is not specified, only the first 
+			security module asking for security registration will be
+			loaded. An invalid security module name will be treated
+			as if no module has been chosen.
+
 	capability.disable=
 			[SECURITY] Disable capabilities.  This would normally
 			be used only if an alternative security model is to be
diff --git a/include/linux/security.h b/include/linux/security.h
index a33fd03..601318a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -42,6 +42,9 @@
 
 extern unsigned securebits;
 
+/* Maximum number of letters for an LSM name string */
+#define SECURITY_NAME_MAX	10
+
 struct ctl_table;
 
 /*
@@ -117,6 +120,12 @@ struct request_sock;
 /**
  * struct security_operations - main security structure
  *
+ * Security module identifier.
+ *
+ * @name:
+ *	A string that acts as a unique identifeir for the LSM with max number 
+ *	of characters = SECURITY_NAME_MAX.
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_alloc_security:
@@ -1218,6 +1227,8 @@ struct request_sock;
  * This is the main security structure.
  */
 struct security_operations {
+	char name[SECURITY_NAME_MAX + 1];
+
 	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
 	int (*capget) (struct task_struct * target,
 		       kernel_cap_t * effective,
@@ -1477,6 +1488,7 @@ struct security_operations {
 
 /* prototypes */
 extern int security_init	(void);
+extern int security_module_enable(struct security_operations *ops);
 extern int register_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
diff --git a/security/dummy.c b/security/dummy.c
index 6a0056b..d0910f2 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -986,7 +986,7 @@ static int dummy_key_getsecurity(struct key *key, char **_buffer)
 
 #endif /* CONFIG_KEYS */
 
-struct security_operations dummy_security_ops;
+struct security_operations dummy_security_ops = { "dummy" };
 
 #define set_to_dummy_if_null(ops, function)				\
 	do {								\
diff --git a/security/security.c b/security/security.c
index 3e75b90..6f2df1a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -17,6 +17,9 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 
+/* Boot-time LSM user choice */
+static char chosen_lsm[SECURITY_NAME_MAX + 1];
+static atomic_t security_ops_enabled = ATOMIC_INIT(-1);
 
 /* things that live in dummy.c */
 extern struct security_operations dummy_security_ops;
@@ -30,7 +33,7 @@ unsigned long mmap_min_addr = CONFIG_SECURITY_DEFAULT_MMAP_MIN_ADDR;
 static inline int verify(struct security_operations *ops)
 {
 	/* verify the security_operations structure exists */
-	if (!ops)
+	if (!ops || !ops->name)
 		return -EINVAL;
 	security_fixup_ops(ops);
 	return 0;
@@ -67,13 +70,51 @@ int __init security_init(void)
 	return 0;
 }
 
+/* Save user chosen LSM */
+static int __init choose_lsm(char *str)
+{
+	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	chosen_lsm[SECURITY_NAME_MAX] = NULL;
+
+	return 1;
+}
+__setup("security=", choose_lsm);
+
+/**
+ * security_module_enable - Load given security module on boot ?
+ * @ops: a pointer to the struct security_operations that is to be checked.
+ *
+ * Each LSM must pass this method before registering its own operations
+ * to avoid security registration races.
+ *
+ * Return true if:
+ *	-The passed LSM is the one chosen by user at boot time,
+ *	-or user didsn't specify a specific LSM and we're the first to ask
+ *	 for registeration permissoin.
+ * Otherwise, return false.
+ */
+int security_module_enable(struct security_operations *ops)
+{
+	if (!ops || !ops->name)
+		return 0;
+
+	if (!*chosen_lsm && atomic_inc_and_test(&security_ops_enabled))
+		return 1;
+
+	if (!strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
+		return 1;
+	
+	return 0;
+}
+
 /**
  * register_security - registers a security framework with the kernel
  * @ops: a pointer to the struct security_options that is to be registered
  *
  * This function is to allow a security module to register itself with the
  * kernel security subsystem.  Some rudimentary checking is done on the @ops
- * value passed to this function.
+ * value passed to this function. You'll need to check first if your LSM
+ * is allowed to register its @ops by calling security_module_enable(@ops).
  *
  * If there is already a security module registered with the kernel,
  * an error will be returned.  Otherwise 0 is returned on success.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f42ebfc..b507977 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5233,6 +5233,8 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_operations selinux_ops = {
+	.name =				"selinux",
+
 	.ptrace =			selinux_ptrace,
 	.capget =			selinux_capget,
 	.capset_check =			selinux_capset_check,
@@ -5425,6 +5427,15 @@ static __init int selinux_init(void)
 		return 0;
 	}
 
+	if (!security_module_enable(&selinux_ops)) {
+		selinux_enabled = 0;
+		printk(KERN_INFO "SELinux:  Security registration was not allowed.\n");
+		printk(KERN_INFO "SELinux:  Another security module was chosen.\n");
+		printk(KERN_INFO "SELinux:  Use security=%s to force choosing "
+		       "SELinux on boot.\n", selinux_ops.name);
+		return 0;
+	}
+
 	printk(KERN_INFO "SELinux:  Initializing.\n");
 
 	/* Set the security state for the initial task. */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b5d6f7..2e1adbb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 }
 
 static struct security_operations smack_ops = {
+	.name =				"smack",
+
 	.ptrace = 			smack_ptrace,
 	.capget = 			cap_capget,
 	.capset_check = 		cap_capset_check,
@@ -2485,6 +2487,14 @@ static struct security_operations smack_ops = {
  */
 static __init int smack_init(void)
 {
+	if (!security_module_enable(&smack_ops)) {
+		printk(KERN_INFO "Smack:  Security registration was not allowed.\n");
+		printk(KERN_INFO "Smack:  Another security module was chosen.\n");
+		printk(KERN_INFO "Smack:  Use security=%s to force choosing "
+		       "Smack on boot.\n", smack_ops.name);
+		return 0;
+	}
+
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
 	/*

---
"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v3 -mm] LSM: Add security= boot parameter

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi!,
> 
> [
> Fixed two bugs:
>        - concurrency: incrementing and testing atomic_t in different places.
>        - overflow: not ending string with NULL after using strncpy().
>        - I'll never write a patch when I'm asleep, sorry :(
> 
> Added more verbose messages to SMACK and SELinux if they were not 
> chosen on boot.
> 
> Casey: Failing to take permission to register an LSM does not mean that 
>        the other has registered its security_ops yet. It just means that
>        the other asked for allowance to call register_security(). It's 
>        not yet guraranteed that this registration succeeded.
> 
>        This means that adding "SELinux: failed to load, LSM %s is loaded"
>        may lead to %s = "dummy" in case of a highly concurrent SMP system.
> ]

Personally, I'd be OK with seeing "dummy" on my Altix on occasion. :-)
Perhaps "SELinux: Not registered, %s is reported" would address the
concern. It would be really good to see the value in the 99 44/100%
of the cases where it is available, even if it means admitting that
there are limited circumstances where you might know that someone
got there ahead of you, but not who it was. I don't think it's
worth going to heroic efforts to make sure it's available.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH -v3 -mm] LSM: Add security= boot parameter

[James Morris]

On Sun, 2 Mar 2008, Ahmed S. Darwish wrote:

> Add the security= boot parameter. This is done to avoid LSM 
> registration clashes in case of more than one bult-in module.
> 
> User can choose a security module to enable at boot. If no 
> security= boot parameter is specified, only the first LSM 
> asking for registration will be loaded. An invalid security 
> module name will be treated as if no module has been chosen.
> 
> LSM modules must check now if they are allowed to register
> by calling security_module_enable(ops) first. Modify SELinux 
> and SMACK to do so.

I think this can be simplified by folding the logic into 
register_security(), rather than having a two-stage LSM registration 
process.

So, this function would now look like

	int register_security(ops, *status);

and set *status to LSM_WAS_CHOSEN (or similar) if the module being 
registered was also chosen via the security= parameter.  If there is no 
value for the parameter, the first module to register is automatically 
chosen, to preserve existing behavior.

The calling code can then decide what to do, e.g. not panic if 
registration failed and the LSM was not chosen; panic on failure when it 
was chosen.

> +static atomic_t security_ops_enabled = ATOMIC_INIT(-1);

I'd suggest getting rid of this atomic and using a spinlock to protect the 
global chosen_lsm string, which is always filled when an LSM registers.

>  
> +/* Save user chosen LSM */
> +static int __init choose_lsm(char *str)
> +{
> +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> +	chosen_lsm[SECURITY_NAME_MAX] = NULL;

You should never need to set the last byte to NULL -- it's initialized to 
that and by definition should never be overwritten.

> +int security_module_enable(struct security_operations *ops)
> +{
> +	if (!ops || !ops->name)
> +		return 0;

Lack of ops->name during registration needs to be a BUG_ON.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH -v3 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

Hi James,

On Mon, Mar 03, 2008 at 07:29:22PM +1100, James Morris wrote:
> On Sun, 2 Mar 2008, Ahmed S. Darwish wrote:
> 
> > Add the security= boot parameter. This is done to avoid LSM 
> > registration clashes in case of more than one bult-in module.
> > 
> > User can choose a security module to enable at boot. If no 
> > security= boot parameter is specified, only the first LSM 
> > asking for registration will be loaded. An invalid security 
> > module name will be treated as if no module has been chosen.
> > 
> > LSM modules must check now if they are allowed to register
> > by calling security_module_enable(ops) first. Modify SELinux 
> > and SMACK to do so.
> 
> I think this can be simplified by folding the logic into 
> register_security(), rather than having a two-stage LSM registration 
> process.
> 
> So, this function would now look like
> 
> 	int register_security(ops, *status);
> 
> and set *status to LSM_WAS_CHOSEN (or similar) if the module being 
> registered was also chosen via the security= parameter.  If there is no 
> value for the parameter, the first module to register is automatically 
> chosen, to preserve existing behavior.
> 
> The calling code can then decide what to do, e.g. not panic if 
> registration failed and the LSM was not chosen; panic on failure when it 
> was chosen.
> 

I liked to do it like that at first, but I faced two problems:

SElinux (As you already know ;)) does the security setup of the initial 
task before calling register_security. Would it be safe to do this
setup after registeration ?

Same case occurs for Smack, it does some locking initializations and
setup initial task's security before registration.

Personally, I feel that it's nicer to let the LSM know if it's
OK to initialize itself before hitting _the point of no return_ (registration).

Anyway, I have no problem to implement it using *status if my 
concerns are wrong.

> > +static atomic_t security_ops_enabled = ATOMIC_INIT(-1);
> 
> I'd suggest getting rid of this atomic and using a spinlock to protect the 
> global chosen_lsm string, which is always filled when an LSM registers.
> 
> >  
> > +/* Save user chosen LSM */
> > +static int __init choose_lsm(char *str)
> > +{
> > +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> > +	chosen_lsm[SECURITY_NAME_MAX] = NULL;
> 
> You should never need to set the last byte to NULL -- it's initialized to 
> that and by definition should never be overwritten.
> 
> > +int security_module_enable(struct security_operations *ops)
> > +{
> > +	if (!ops || !ops->name)
> > +		return 0;
> 
> Lack of ops->name during registration needs to be a BUG_ON.
> 

You'll find above three points fixed the next send. Thank you.

Regards,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v3 -mm] LSM: Add security= boot parameter

[Stephen Smalley]

On Mon, 2008-03-03 at 17:35 +0200, Ahmed S. Darwish wrote:
> Hi James,
> 
> On Mon, Mar 03, 2008 at 07:29:22PM +1100, James Morris wrote:
> > On Sun, 2 Mar 2008, Ahmed S. Darwish wrote:
> > 
> > > Add the security= boot parameter. This is done to avoid LSM 
> > > registration clashes in case of more than one bult-in module.
> > > 
> > > User can choose a security module to enable at boot. If no 
> > > security= boot parameter is specified, only the first LSM 
> > > asking for registration will be loaded. An invalid security 
> > > module name will be treated as if no module has been chosen.
> > > 
> > > LSM modules must check now if they are allowed to register
> > > by calling security_module_enable(ops) first. Modify SELinux 
> > > and SMACK to do so.
> > 
> > I think this can be simplified by folding the logic into 
> > register_security(), rather than having a two-stage LSM registration 
> > process.
> > 
> > So, this function would now look like
> > 
> > 	int register_security(ops, *status);
> > 
> > and set *status to LSM_WAS_CHOSEN (or similar) if the module being 
> > registered was also chosen via the security= parameter.  If there is no 
> > value for the parameter, the first module to register is automatically 
> > chosen, to preserve existing behavior.
> > 
> > The calling code can then decide what to do, e.g. not panic if 
> > registration failed and the LSM was not chosen; panic on failure when it 
> > was chosen.
> > 
> 
> I liked to do it like that at first, but I faced two problems:
> 
> SElinux (As you already know ;)) does the security setup of the initial 
> task before calling register_security. Would it be safe to do this
> setup after registeration ?

I wouldn't recommend it - the hook functions presume that the initial
task security blob has been set up already, and that other dependencies
like the inode security cache and access vector cache have been created
and can be used.  We have to assume that the security hooks can start
being invoked as soon as we call register_security(), even if in
practice it won't happen until after the init function completes.

> Same case occurs for Smack, it does some locking initializations and
> setup initial task's security before registration.
> 
> Personally, I feel that it's nicer to let the LSM know if it's
> OK to initialize itself before hitting _the point of no return_ (registration).
> 
> Anyway, I have no problem to implement it using *status if my 
> concerns are wrong.
> 
> > > +static atomic_t security_ops_enabled = ATOMIC_INIT(-1);
> > 
> > I'd suggest getting rid of this atomic and using a spinlock to protect the 
> > global chosen_lsm string, which is always filled when an LSM registers.
> > 
> > >  
> > > +/* Save user chosen LSM */
> > > +static int __init choose_lsm(char *str)
> > > +{
> > > +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> > > +	chosen_lsm[SECURITY_NAME_MAX] = NULL;
> > 
> > You should never need to set the last byte to NULL -- it's initialized to 
> > that and by definition should never be overwritten.
> > 
> > > +int security_module_enable(struct security_operations *ops)
> > > +{
> > > +	if (!ops || !ops->name)
> > > +		return 0;
> > 
> > Lack of ops->name during registration needs to be a BUG_ON.
> > 
> 
> You'll find above three points fixed the next send. Thank you.
> 
> Regards,
> 
-- 
Stephen Smalley
National Security Agency

[PATCH -v4 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

Hi!,

On Mon, Mar 03, 2008 at 10:54:02AM -0500, Stephen Smalley wrote:
> 
> On Mon, 2008-03-03 at 17:35 +0200, Ahmed S. Darwish wrote:
> > Hi James,
> > 
....
> > 
> > SElinux (As you already know ;)) does the security setup of the initial 
> > task before calling register_security. Would it be safe to do this
> > setup after registeration ?
> 
> I wouldn't recommend it - the hook functions presume that the initial
> task security blob has been set up already, and that other dependencies
> like the inode security cache and access vector cache have been created
> and can be used.  We have to assume that the security hooks can start
> being invoked as soon as we call register_security(), even if in
> practice it won't happen until after the init function completes.
>

OK, the patch will stick with the two-stage registration model to give
a safe initialization time for LSMs.

james: Could you please make it OK to use the atomic counter instead of
       the spinlock ? It'll just add unneeded complexity. We only set
       `chosen_lsm' once at the parameters parsing time. 

Changes (per James suggestions):
- Added a BUG_ON(!ops->name).
- Removed a redundant setting of last string character to NULL.

Sample Output:
[    0.065487] SELinux:  Initializing.
[    0.067115] SELinux:  Starting in permissive mode
[    0.067186] Smack:  Another security module was chosen.
[    0.067345] Smack:  Use security=smack to force loading Smack on boot.

-->

Add the security= boot parameter. This is done to avoid LSM 
registration clashes in case of more than one bult-in module.

User can choose a security module to enable at boot. If no 
security= boot parameter is specified, only the first LSM 
asking for registration will be loaded. An invalid security 
module name will be treated as if no module has been chosen.

LSM modules must check now if they are allowed to register
by calling security_module_enable(ops) first. Modify SELinux 
and SMACK to do so.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 Documentation/kernel-parameters.txt |    6 ++++
 include/linux/security.h            |   12 +++++++++
 security/dummy.c                    |    2 -+
 security/security.c                 |   45 ++++++++++++++++++++++++++++++++++--
 security/selinux/hooks.c            |   10 ++++++++
 security/smack/smack_lsm.c          |    9 +++++++

 6 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c64dfd7..85044e8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -374,6 +374,12 @@ and is between 256 and 4096 characters. It is defined in the file
 			possible to determine what the correct size should be.
 			This option provides an override for these situations.
 
+	security=	[SECURITY] Choose a security module to enable at boot. 
+			If this boot parameter is not specified, only the first 
+			security module asking for security registration will be
+			loaded. An invalid security module name will be treated
+			as if no module has been chosen.
+
 	capability.disable=
 			[SECURITY] Disable capabilities.  This would normally
 			be used only if an alternative security model is to be
diff --git a/include/linux/security.h b/include/linux/security.h
index eb663e5..3db2819 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -42,6 +42,9 @@
 
 extern unsigned securebits;
 
+/* Maximum number of letters for an LSM name string */
+#define SECURITY_NAME_MAX	10
+
 struct ctl_table;
 struct audit_krule;
 
@@ -118,6 +121,12 @@ struct request_sock;
 /**
  * struct security_operations - main security structure
  *
+ * Security module identifier.
+ *
+ * @name:
+ *	A string that acts as a unique identifeir for the LSM with max number
+ *	of characters = SECURITY_NAME_MAX.
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_alloc_security:
@@ -1262,6 +1271,8 @@ struct request_sock;
  * This is the main security structure.
  */
 struct security_operations {
+	char name[SECURITY_NAME_MAX + 1];
+
 	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
 	int (*capget) (struct task_struct * target,
 		       kernel_cap_t * effective,
@@ -1530,6 +1541,7 @@ struct security_operations {
 
 /* prototypes */
 extern int security_init	(void);
+extern int security_module_enable(struct security_operations *ops);
 extern int register_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
diff --git a/security/dummy.c b/security/dummy.c
index 241ab20..d081699 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -1022,7 +1022,7 @@ static inline void dummy_audit_rule_free(void *lsmrule)
 
 #endif /* CONFIG_AUDIT */
 
-struct security_operations dummy_security_ops;
+struct security_operations dummy_security_ops = { "dummy" };
 
 #define set_to_dummy_if_null(ops, function)				\
 	do {								\
diff --git a/security/security.c b/security/security.c
index 1bf2ee4..5419fec 100644
--- a/security/security.c
+++ b/security/security.c
@@ -17,6 +17,9 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 
+/* Boot-time LSM user choice */
+static char chosen_lsm[SECURITY_NAME_MAX + 1];
+static atomic_t security_ops_enabled = ATOMIC_INIT(-1);
 
 /* things that live in dummy.c */
 extern struct security_operations dummy_security_ops;
@@ -30,7 +33,7 @@ unsigned long mmap_min_addr = CONFIG_SECURITY_DEFAULT_MMAP_MIN_ADDR;
 static inline int verify(struct security_operations *ops)
 {
 	/* verify the security_operations structure exists */
-	if (!ops)
+	if (!ops || !ops->name)
 		return -EINVAL;
 	security_fixup_ops(ops);
 	return 0;
@@ -67,13 +70,51 @@ int __init security_init(void)
 	return 0;
 }
 
+/* Save user chosen LSM */
+static int __init choose_lsm(char *str)
+{
+	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	return 1;
+}
+__setup("security=", choose_lsm);
+
+/**
+ * security_module_enable - Load given security module on boot ?
+ * @ops: a pointer to the struct security_operations that is to be checked.
+ *
+ * Each LSM must pass this method before registering its own operations
+ * to avoid security registration races.
+ *
+ * Return true if:
+ *	-The passed LSM is the one chosen by user at boot time,
+ *	-or user didsn't specify a specific LSM and we're the first to ask
+ *	 for registeration permissoin.
+ * Otherwise, return false.
+ */
+int security_module_enable(struct security_operations *ops)
+{
+	if (!ops || !ops->name) {
+		BUG();
+		return 0;
+	}
+
+	if (!*chosen_lsm && atomic_inc_and_test(&security_ops_enabled))
+		return 1;
+
+	if (!strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
+		return 1;
+
+	return 0;
+}
+
 /**
  * register_security - registers a security framework with the kernel
  * @ops: a pointer to the struct security_options that is to be registered
  *
  * This function is to allow a security module to register itself with the
  * kernel security subsystem.  Some rudimentary checking is done on the @ops
- * value passed to this function.
+ * value passed to this function. You'll need to check first if your LSM
+ * is allowed to register its @ops by calling security_module_enable(@ops).
  *
  * If there is already a security module registered with the kernel,
  * an error will be returned.  Otherwise 0 is returned on success.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bef1834..c3a52a8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5247,6 +5247,8 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_operations selinux_ops = {
+	.name =				"selinux",
+
 	.ptrace =			selinux_ptrace,
 	.capget =			selinux_capget,
 	.capset_check =			selinux_capset_check,
@@ -5448,6 +5450,14 @@ static __init int selinux_init(void)
 		return 0;
 	}
 
+	if (!security_module_enable(&selinux_ops)) {
+		selinux_enabled = 0;
+		printk(KERN_INFO "SELinux:  Another security module was chosen.\n");
+		printk(KERN_INFO "SELinux:  Use security=%s to force loading "
+		       "SELinux on boot.\n", selinux_ops.name);
+		return 0;
+	}
+
 	printk(KERN_INFO "SELinux:  Initializing.\n");
 
 	/* Set the security state for the initial task. */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b5d6f7..ad31819 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 }
 
 static struct security_operations smack_ops = {
+	.name =				"smack",
+
 	.ptrace = 			smack_ptrace,
 	.capget = 			cap_capget,
 	.capset_check = 		cap_capset_check,
@@ -2485,6 +2487,13 @@ static struct security_operations smack_ops = {
  */
 static __init int smack_init(void)
 {
+	if (!security_module_enable(&smack_ops)) {
+		printk(KERN_INFO "Smack:  Another security module was chosen.\n");
+		printk(KERN_INFO "Smack:  Use security=%s to force loading "
+		       "Smack on boot.\n", smack_ops.name);
+		return 0;
+	}
+
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
 	/*

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v4 -mm] LSM: Add security= boot parameter

[James Morris]

On Mon, 3 Mar 2008, Ahmed S. Darwish wrote:

>  static inline int verify(struct security_operations *ops)
>  {
>  	/* verify the security_operations structure exists */
> -	if (!ops)
> +	if (!ops || !ops->name)
>  		return -EINVAL;

verify() will now be called after ops->name has been referenced, so these 
checks won't be necessary now.

> +int security_module_enable(struct security_operations *ops)
> +{
> +	if (!ops || !ops->name) {
> +		BUG();
> +		return 0;
> +	}

It's not going to return after BUG(), and actually, you can probably just 
rely on the subsequent oops (i.e. no check needed).

> +
> +	if (!*chosen_lsm && atomic_inc_and_test(&security_ops_enabled))
> +		return 1;
> +
> +	if (!strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> +		return 1;


I still think you should use a spinlock here to make the semantics 
simpler.  Dispense with the confusingly named security_ops_enabled, and 
fill chosen_lsm in with the first lsm to regsiter if none chosen at boot.


> +               printk(KERN_INFO "SELinux:  Another security module was chosen.\n");
> +               printk(KERN_INFO "SELinux:  Use security=%s to force loading "
> +                      "SELinux on boot.\n", selinux_ops.name);

These messages are not going to scale, e.g. what if there are 20 LSMs 
compiled in, and they all print this on boot? Just print the chosen LSM 
in security_module_enabled().


- James
-- 
James Morris
<jmorris@namei.org>

[PATCH -v5 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

Hi!,

[ 
Fix stuff mentioned by James in parent mail:
- use spinlocks instead of atomic counter (yes, this is clearer).
- remove redundant BUG_ON
- don't let LSMs loudly complain when they aren't chosen.
]

-->

Add the security= boot parameter. This is done to avoid LSM 
registration clashes in case of more than one bult-in module.

User can choose a security module to enable at boot. If no 
security= boot parameter is specified, only the first LSM 
asking for registration will be loaded. An invalid security 
module name will be treated as if no module has been chosen.

LSM modules must check now if they are allowed to register
by calling security_module_enable(ops) first. Modify SELinux 
and SMACK to do so.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

 Documentation/kernel-parameters.txt |    6 ++++
 include/linux/security.h            |   12 +++++++++
 security/dummy.c                    |    2 -
 security/security.c                 |   46 +++++++++++++++++++++++++++++++++++-
 security/selinux/hooks.c            |    7 +++++
 security/smack/smack_lsm.c          |    5 +++

 6 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c64dfd7..85044e8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -374,6 +374,12 @@ and is between 256 and 4096 characters. It is defined in the file
 			possible to determine what the correct size should be.
 			This option provides an override for these situations.
 
+	security=	[SECURITY] Choose a security module to enable at boot. 
+			If this boot parameter is not specified, only the first 
+			security module asking for security registration will be
+			loaded. An invalid security module name will be treated
+			as if no module has been chosen.
+
 	capability.disable=
 			[SECURITY] Disable capabilities.  This would normally
 			be used only if an alternative security model is to be
diff --git a/include/linux/security.h b/include/linux/security.h
index eb663e5..3db2819 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -42,6 +42,9 @@
 
 extern unsigned securebits;
 
+/* Maximum number of letters for an LSM name string */
+#define SECURITY_NAME_MAX	10
+
 struct ctl_table;
 struct audit_krule;
 
@@ -118,6 +121,12 @@ struct request_sock;
 /**
  * struct security_operations - main security structure
  *
+ * Security module identifier.
+ *
+ * @name:
+ *	A string that acts as a unique identifeir for the LSM with max number
+ *	of characters = SECURITY_NAME_MAX.
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_alloc_security:
@@ -1262,6 +1271,8 @@ struct request_sock;
  * This is the main security structure.
  */
 struct security_operations {
+	char name[SECURITY_NAME_MAX + 1];
+
 	int (*ptrace) (struct task_struct * parent, struct task_struct * child);
 	int (*capget) (struct task_struct * target,
 		       kernel_cap_t * effective,
@@ -1530,6 +1541,7 @@ struct security_operations {
 
 /* prototypes */
 extern int security_init	(void);
+extern int security_module_enable(struct security_operations *ops);
 extern int register_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern struct dentry *securityfs_create_file(const char *name, mode_t mode,
diff --git a/security/dummy.c b/security/dummy.c
index 241ab20..d081699 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -1022,7 +1022,7 @@ static inline void dummy_audit_rule_free(void *lsmrule)
 
 #endif /* CONFIG_AUDIT */
 
-struct security_operations dummy_security_ops;
+struct security_operations dummy_security_ops = { "dummy" };
 
 #define set_to_dummy_if_null(ops, function)				\
 	do {								\
diff --git a/security/security.c b/security/security.c
index 1bf2ee4..def9fc0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -17,6 +17,9 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 
+/* Boot-time LSM user choice */
+static spinlock_t chosen_lsm_lock;
+static char chosen_lsm[SECURITY_NAME_MAX + 1];
 
 /* things that live in dummy.c */
 extern struct security_operations dummy_security_ops;
@@ -62,18 +65,59 @@ int __init security_init(void)
 	}
 
 	security_ops = &dummy_security_ops;
+	spin_lock_init(&chosen_lsm_lock);
 	do_security_initcalls();
 
 	return 0;
 }
 
+/* Save user chosen LSM */
+static int __init choose_lsm(char *str)
+{
+	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	return 1;
+}
+__setup("security=", choose_lsm);
+
+/**
+ * security_module_enable - Load given security module on boot ?
+ * @ops: a pointer to the struct security_operations that is to be checked.
+ *
+ * Each LSM must pass this method before registering its own operations
+ * to avoid security registration races.
+ *
+ * Return true if:
+ *	-The passed LSM is the one chosen by user at boot time,
+ *	-or user didsn't specify a specific LSM and we're the first to ask
+ *	 for registeration permissoin.
+ * Otherwise, return false.
+ */
+int security_module_enable(struct security_operations *ops)
+{
+	int rc = 1;
+
+	spin_lock(&chosen_lsm_lock);
+	if (!*chosen_lsm)
+		strncpy(chosen_lsm, ops->name, SECURITY_NAME_MAX);
+	else if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
+		rc = 0;
+	spin_unlock(&chosen_lsm_lock);
+
+	if (rc)
+		printk(KERN_INFO "Security: Loading '%s' security module.\n",
+		       ops->name);
+
+	return rc;
+}
+
 /**
  * register_security - registers a security framework with the kernel
  * @ops: a pointer to the struct security_options that is to be registered
  *
  * This function is to allow a security module to register itself with the
  * kernel security subsystem.  Some rudimentary checking is done on the @ops
- * value passed to this function.
+ * value passed to this function. You'll need to check first if your LSM
+ * is allowed to register its @ops by calling security_module_enable(@ops).
  *
  * If there is already a security module registered with the kernel,
  * an error will be returned.  Otherwise 0 is returned on success.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bef1834..52aaf10 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5247,6 +5247,8 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_operations selinux_ops = {
+	.name =				"selinux",
+
 	.ptrace =			selinux_ptrace,
 	.capget =			selinux_capget,
 	.capset_check =			selinux_capset_check,
@@ -5443,6 +5445,11 @@ static __init int selinux_init(void)
 {
 	struct task_security_struct *tsec;
 
+	if (!security_module_enable(&selinux_ops)) {
+		selinux_enabled = 0;
+		return 0;
+	}
+
 	if (!selinux_enabled) {
 		printk(KERN_INFO "SELinux:  Disabled at boot.\n");
 		return 0;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b5d6f7..9464185 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2358,6 +2358,8 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 }
 
 static struct security_operations smack_ops = {
+	.name =				"smack",
+
 	.ptrace = 			smack_ptrace,
 	.capget = 			cap_capget,
 	.capset_check = 		cap_capset_check,
@@ -2485,6 +2487,9 @@ static struct security_operations smack_ops = {
  */
 static __init int smack_init(void)
 {
+	if (!security_module_enable(&smack_ops))
+		return 0;
+
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
 	/*

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

[James Morris]

On Tue, 4 Mar 2008, Ahmed S. Darwish wrote:

> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>

Acked-by: James Morris <jmorris@namei.org>

> +	int rc = 1;
> +
> +	spin_lock(&chosen_lsm_lock);
> +	if (!*chosen_lsm)
> +		strncpy(chosen_lsm, ops->name, SECURITY_NAME_MAX);
> +	else if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> +		rc = 0;
> +	spin_unlock(&chosen_lsm_lock);
> +
> +	if (rc)
> +		printk(KERN_INFO "Security: Loading '%s' security module.\n",
> +		       ops->name);
> +
> +	return rc;

Possibly consider using 0 for success and -EBUSY on failure (but not a 
showstopper for me, as it's not really an "error").


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

[Andrew Morton]

On Tue, 4 Mar 2008 05:04:07 +0200
"Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi!,
> 
> [ 
> Fix stuff mentioned by James in parent mail:
> - use spinlocks instead of atomic counter (yes, this is clearer).
> - remove redundant BUG_ON
> - don't let LSMs loudly complain when they aren't chosen.
> ]
> 
> -->
> 
> Add the security= boot parameter. This is done to avoid LSM 
> registration clashes in case of more than one bult-in module.
> 
> User can choose a security module to enable at boot. If no 
> security= boot parameter is specified, only the first LSM 
> asking for registration will be loaded. An invalid security 
> module name will be treated as if no module has been chosen.
> 
> LSM modules must check now if they are allowed to register
> by calling security_module_enable(ops) first. Modify SELinux 
> and SMACK to do so.
> 
> ...
>  
> +/* Maximum number of letters for an LSM name string */
> +#define SECURITY_NAME_MAX	10

Is this long enough?

>  struct ctl_table;
>  struct audit_krule;
>  
>  ...
>
> -struct security_operations dummy_security_ops;
> +struct security_operations dummy_security_ops = { "dummy" };

Please don't rely upon the layout of data structures in this manner.  Use
".name = ".

>  
>  #define set_to_dummy_if_null(ops, function)				\
>  	do {								\
> diff --git a/security/security.c b/security/security.c
> index 1bf2ee4..def9fc0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -17,6 +17,9 @@
>  #include <linux/kernel.h>
>  #include <linux/security.h>
>  
> +/* Boot-time LSM user choice */
> +static spinlock_t chosen_lsm_lock;
> +static char chosen_lsm[SECURITY_NAME_MAX + 1];
>  
>  /* things that live in dummy.c */
>  extern struct security_operations dummy_security_ops;
> @@ -62,18 +65,59 @@ int __init security_init(void)
>  	}
>  
>  	security_ops = &dummy_security_ops;
> +	spin_lock_init(&chosen_lsm_lock);

Please remove this and use compile-time initialisation with DEFINE_SPINLOCK.

Do we actually need the lock?  This code is only called at boot-time if I
understand it correctly?

Can chosen_lsm[] be __initdata?

>  	do_security_initcalls();
>  
>  	return 0;
>  }
>  
> +/* Save user chosen LSM */
> +static int __init choose_lsm(char *str)
> +{
> +	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
> +	return 1;
> +}
> +__setup("security=", choose_lsm);
> +
> +/**
> + * security_module_enable - Load given security module on boot ?
> + * @ops: a pointer to the struct security_operations that is to be checked.
> + *
> + * Each LSM must pass this method before registering its own operations
> + * to avoid security registration races.
> + *
> + * Return true if:
> + *	-The passed LSM is the one chosen by user at boot time,
> + *	-or user didsn't specify a specific LSM and we're the first to ask
> + *	 for registeration permissoin.
> + * Otherwise, return false.
> + */
> +int security_module_enable(struct security_operations *ops)
> +{
> +	int rc = 1;
> +
> +	spin_lock(&chosen_lsm_lock);
> +	if (!*chosen_lsm)
> +		strncpy(chosen_lsm, ops->name, SECURITY_NAME_MAX);
> +	else if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> +		rc = 0;
> +	spin_unlock(&chosen_lsm_lock);
> +
> +	if (rc)
> +		printk(KERN_INFO "Security: Loading '%s' security module.\n",
> +		       ops->name);
> +
> +	return rc;
> +}

I believe this can be __init.

> +	if (!security_module_enable(&selinux_ops)) {
> +		selinux_enabled = 0;
> +		return 0;
> +	}
> +
>
> ...
>
>  static __init int smack_init(void)
>  {
> +	if (!security_module_enable(&smack_ops))
> +		return 0;
> +
>  	printk(KERN_INFO "Smack:  Initializing.\n");
>  
>  	/*

hm.  selinux has a global selinux_enabled knob, but smack seems to be able
to get by without one.  +1 for smack ;)

Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

On Wed, Mar 05, 2008 at 02:29:48PM -0800, Andrew Morton wrote:
> On Tue, 4 Mar 2008 05:04:07 +0200
> "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> 
...
> > ...
> >  
> > +/* Maximum number of letters for an LSM name string */
> > +#define SECURITY_NAME_MAX	10
> 
> Is this long enough?
> 

I've judged from the four common applicants (selinux, smack,
apparmor, tomoyo) that 10 would be enough. Anyway this will be
easy to fix when something longer appears.

> >  struct ctl_table;
> >  struct audit_krule;
> >  
> >  ...
> >
> > -struct security_operations dummy_security_ops;
> > +struct security_operations dummy_security_ops = { "dummy" };
> 
> Please don't rely upon the layout of data structures in this manner.  Use
> ".name = ".
> 

Will do.

> >  
> >  #define set_to_dummy_if_null(ops, function)				\
> >  	do {								\
> > diff --git a/security/security.c b/security/security.c
> > index 1bf2ee4..def9fc0 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -17,6 +17,9 @@
> >  #include <linux/kernel.h>
> >  #include <linux/security.h>
> >  
> > +/* Boot-time LSM user choice */
> > +static spinlock_t chosen_lsm_lock;
> > +static char chosen_lsm[SECURITY_NAME_MAX + 1];
> >  
> >  /* things that live in dummy.c */
> >  extern struct security_operations dummy_security_ops;
> > @@ -62,18 +65,59 @@ int __init security_init(void)
> >  	}
> >  
> >  	security_ops = &dummy_security_ops;
> > +	spin_lock_init(&chosen_lsm_lock);
> 
> Please remove this and use compile-time initialisation with DEFINE_SPINLOCK.
> 

Ooh I thought the dynamic one was better cause I remember I read it
somewhere on LWN that this is nicer for the RT-patches. I'll modify
it, no problem.

> Do we actually need the lock?  This code is only called at boot-time if I
> understand it correctly?
> 

In the latest version (-v7b, in another thread, CCed), security_module_enable()
is also used to let an LSM know if it's currently loaded or not. This
was done to avoid using a `smack_enabled' global.

I'll resend the v7b for -mm once the LSM devs give their ACKs for the
-rc3 one.

> Can chosen_lsm[] be __initdata?
> 

You're the expert ;), I don't really understand the difference.

...
> >
> > +/**
> > + * security_module_enable - Load given security module on boot ?
> > + * @ops: a pointer to the struct security_operations that is to be checked.
> > + *
> > + * Each LSM must pass this method before registering its own operations
> > + * to avoid security registration races.
> > + *
> > + * Return true if:
> > + *	-The passed LSM is the one chosen by user at boot time,
> > + *	-or user didsn't specify a specific LSM and we're the first to ask
> > + *	 for registeration permissoin.
> > + * Otherwise, return false.
> > + */
> > +int security_module_enable(struct security_operations *ops)
> > +{
> > +	int rc = 1;
> > +
> > +	spin_lock(&chosen_lsm_lock);
> > +	if (!*chosen_lsm)
> > +		strncpy(chosen_lsm, ops->name, SECURITY_NAME_MAX);
> > +	else if (strncmp(ops->name, chosen_lsm, SECURITY_NAME_MAX))
> > +		rc = 0;
> > +	spin_unlock(&chosen_lsm_lock);
> > +
> > +	if (rc)
> > +		printk(KERN_INFO "Security: Loading '%s' security module.\n",
> > +		       ops->name);
> > +
> > +	return rc;
> > +}
> 
> I believe this can be __init.
> 

Will do.

> > +	if (!security_module_enable(&selinux_ops)) {
> > +		selinux_enabled = 0;
> > +		return 0;
> > +	}
> > +
> >
> > ...
> >
> >  static __init int smack_init(void)
> >  {
> > +	if (!security_module_enable(&smack_ops))
> > +		return 0;
> > +
> >  	printk(KERN_INFO "Smack:  Initializing.\n");
> >  
> >  	/*
> 
> hm.  selinux has a global selinux_enabled knob, but smack seems to be able
> to get by without one.  +1 for smack ;)
> 

Thanks to Linus ;). 

I've sent a patch that added a similar global yesterday and it was 
knocked-down by Linus after exactly 2 minutes. 
The situation is handled now without a global in v7/v7b.

Regards,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

[James Morris]

On Wed, 5 Mar 2008, Andrew Morton wrote:

> > +/* Maximum number of letters for an LSM name string */
> > +#define SECURITY_NAME_MAX	10
> 
> Is this long enough?

I almost flagged this earlier, but I don't think we've ever seen an LSM 
with a longer name, and it can be expanded if needed.  32 or something 
seems similarly arbitrary.

> Please remove this and use compile-time initialisation with DEFINE_SPINLOCK.
> 
> Do we actually need the lock?  This code is only called at boot-time if I
> understand it correctly?

Theoretically, security_module_enable() could be called at any time, 
although it does seem unlikely never to be called at boot, especially if 
multiple LSMs are compiled in.

In that case, perhaps mark the function as __init, and require it be 
called only at boot time.

> Can chosen_lsm[] be __initdata?

With the above, yes.

> > +int security_module_enable(struct security_operations *ops)
> > +}
> 
> I believe this can be __init.

Indeed :-)


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

[Ahmed S. Darwish]

On Thu, Mar 06, 2008 at 12:56:28AM +0200, Ahmed S. Darwish wrote:
> On Wed, Mar 05, 2008 at 02:29:48PM -0800, Andrew Morton wrote:
> > On Tue, 4 Mar 2008 05:04:07 +0200
> > "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> > 
> > Can chosen_lsm[] be __initdata?
> > 
> 
> You're the expert ;), I don't really understand the difference.
> 

After being a good citizen and reading the comments in init.h, I think
yes it should be marked so. even though we use it from init_smk_fs
since init_smk_fs is also marked as __init.

Thank you,

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com