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

[Andrew Morton <akpm@linux-foundation.org>]

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 ;)