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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080305225628.GA6746@ubuntu>
Date:	Thu, 6 Mar 2008 00:56:28 +0200
From:	"Ahmed S. Darwish" <darwish.07@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	jmorris@...ei.org, sds@...ho.nsa.gov, casey@...aufler-ca.com,
	bunk@...nel.org, chrisw@...s-sol.org, eparis@...isplace.org,
	adobriyan@...ru, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH -v5 -mm] LSM: Add security= boot parameter

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@...il.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/