LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Ahmed S. Darwish" <darwish.07@gmail.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: jmorris@namei.org, sds@tycho.nsa.gov, casey@schaufler-ca.com,
	bunk@kernel.org, chrisw@sous-sol.org, eparis@parisplace.org,
	adobriyan@sw.ru, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH -v5 -mm] LSM: Add security= boot parameter
Date: Thu, 6 Mar 2008 00:56:28 +0200
Message-ID: <20080305225628.GA6746@ubuntu> (raw)
In-Reply-To: <20080305142948.3d391d84.akpm@linux-foundation.org>

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


  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-01 19:07 [RFC PATCH -mm] LSM: Add lsm= " Ahmed S. Darwish
2008-03-01 20:28 ` Casey Schaufler
2008-03-01 21:11   ` Adrian Bunk
2008-03-01 21:29     ` Casey Schaufler
2008-03-01 23:27       ` [PATCH -v2 -mm] LSM: Add security= " Ahmed S. Darwish
2008-03-02  3:41         ` Casey Schaufler
2008-03-02  7:55           ` Ahmed S. Darwish
2008-03-02  7:49         ` Ahmed S. Darwish
2008-03-02 10:59           ` [PATCH -v3 " Ahmed S. Darwish
2008-03-02 18:37             ` Casey Schaufler
2008-03-03  8:29             ` James Morris
2008-03-03 15:35               ` Ahmed S. Darwish
2008-03-03 15:54                 ` Stephen Smalley
2008-03-03 21:24                   ` [PATCH -v4 " Ahmed S. Darwish
2008-03-03 22:16                     ` James Morris
2008-03-04  3:04                       ` [PATCH -v5 " Ahmed S. Darwish
2008-03-04  4:07                         ` James Morris
2008-03-05 22:29                         ` Andrew Morton
2008-03-05 22:56                           ` Ahmed S. Darwish [this message]
2008-03-05 23:06                             ` Ahmed S. Darwish
2008-03-05 22:56                           ` James Morris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080305225628.GA6746@ubuntu \
    --to=darwish.07@gmail.com \
    --cc=adobriyan@sw.ru \
    --cc=akpm@linux-foundation.org \
    --cc=bunk@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=chrisw@sous-sol.org \
    --cc=eparis@parisplace.org \
    --cc=jmorris@namei.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=sds@tycho.nsa.gov \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lkml.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git