Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Casey Schaufler]

----- Original Message ----
> From: Linus Torvalds <torvalds@linux-foundation.org>
> To: Ahmed S. Darwish <darwish.07@gmail.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>; LKML <linux-kernel@vger.kernel.org>
> Sent: Tuesday, March 4, 2008 9:21:19 AM
> Subject: Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded
> 
> 
> 
> On Tue, 4 Mar 2008, Ahmed S. Darwish wrote:
> > 
> > Smackfs initialization without an enabled Smack leads to
> > an early Oops that renders the system unusable.
> 
> I really think this is bogus. Global enables like this are just wrong, and 
> a sign that something else bad is going on.
> 
> What is the oops? Why does it happen?

A kernel that is built with both SELinux and Smack contains
all of the components for both, including smackfs. If SELinux
is chosen as the module to be used and smackfs is initialized
the oops occurs because the Smack initialization that smackfs
depends on has not been done.

One solution would be to tighten the smackfs code so that it
handles the uninitialized LSM case properly.

Another would be to set up Kconfig as to make SELinux and Smack
mutually exclusive, although I really don't know how well that
would go over in testing circles because "config all on" becomes
ambiguous.

A third would be to provide for stacking, but I assume that's
beyond the scope of this exercise.

So I think that fixing up smackfs is the right choice at this point.

> 
> 
> Linus



 
Casey Schaufler
casey@schaufler-ca.com 

Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Linus Torvalds]

On Tue, 4 Mar 2008, Casey Schaufler wrote:
> 
> One solution would be to tighten the smackfs code so that it
> handles the uninitialized LSM case properly.

I really would tend to prefer that.

We had a very similar situation in the ACPI/WMI code, where some WMI 
helper routines would crash just because the WMI data structures hadn't 
been initialized.

The proper fix in that case (in my opinion, and the one that thus got 
committed ;^) was simply to make sure that the data structures were simply 
always consistent (and empty), even if the code itself was disabled. That 
just automatically meant that it didn't need any global flags to be 
tested.

So _if_ the alternative is as simple as just making sure that smackfs 
keeps its data structures consistent even when disabled, I think that's 
the right patch, rather than adding a separate hack to not touch them.

		Linus

Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[James Morris]

On Tue, 4 Mar 2008, Linus Torvalds wrote:

> 
> 
> On Tue, 4 Mar 2008, Casey Schaufler wrote:
> > 
> > One solution would be to tighten the smackfs code so that it
> > handles the uninitialized LSM case properly.
> 
> I really would tend to prefer that.

Can you simply call init_smk_fs() from smack_init() prior to 
register_security() ?


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

Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Ahmed S. Darwish]

On Wed, Mar 05, 2008 at 11:58:08AM +1100, James Morris wrote:
> On Tue, 4 Mar 2008, Linus Torvalds wrote:
> 
> > 
> > 
> > On Tue, 4 Mar 2008, Casey Schaufler wrote:
> > > 
> > > One solution would be to tighten the smackfs code so that it
> > > handles the uninitialized LSM case properly.
> > 
> > I really would tend to prefer that.
> 
> Can you simply call init_smk_fs() from smack_init() prior to 
> register_security() ?
> 

After analysing below oops:

[    0.072989] Call Trace:
[    0.072989]  [<c017d4a4>] alloc_vfsmnt+0x24/0xd0
[    0.072989]  [<c0168ee1>] vfs_kern_mount+0x31/0x120
[    0.072989]  [<c0168fe2>] kern_mount_data+0x12/0x20
[    0.072989]  [<c038a73c>] init_smk_fs+0x4c/0x80
[    0.072989]  [<c038a6ce>] smack_init+0xae/0xd0
[    0.072989]  [<c038a192>] security_init+0x52/0x70
[    0.072989]  [<c037aaa5>] start_kernel+0x1e5/0x290
[    0.072989]  [<c037a380>] unknown_bootoption+0x0/0x1f0
[    0.072989]  =======================

I've found that vfs_caches_init() got called after calling 
security_init() in the main kernel init path. 

This leads to kern_mount() Oopsing cause of Null dereferencing 
of mnt_cache  in the alloc_vfsmnt(mnt_cache, GFP_KERNEL) step.

I was thinking about exporting the current chosen_lsm[] string,
but I don't know if this maybe seen by Linus as another global
that arises from bad design. It may not also scale well if the
LSM needs to check if it's enabled in various places (lots of
strcmp()s).

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 BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Ahmed S. Darwish]

On Tue, Mar 04, 2008 at 09:45:04AM -0800, Casey Schaufler wrote:
> ----- Original Message ----
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > To: Ahmed S. Darwish <darwish.07@gmail.com>
> > Cc: Casey Schaufler <casey@schaufler-ca.com>; LKML <linux-kernel@vger.kernel.org>
> > Sent: Tuesday, March 4, 2008 9:21:19 AM
> > Subject: Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded
> > 
> > 
> > 
> > On Tue, 4 Mar 2008, Ahmed S. Darwish wrote:
> > > 
> > > Smackfs initialization without an enabled Smack leads to
> > > an early Oops that renders the system unusable.
> > 
> > I really think this is bogus. Global enables like this are just wrong, and 
> > a sign that something else bad is going on.
> > 
> > What is the oops? Why does it happen?
...
> 
> One solution would be to tighten the smackfs code so that it
> handles the uninitialized LSM case properly.
> 

IMHO no smackfs code should ever execute if smack isn't loaded.

This means catching it from the very fist step where it registers
itself in init_smk_fs instead of doing several if(we're enabled) cases
in the code path.

The solution should be a _general_ solution, _not_ a SMACK one cause 
SELinux sufferes from exactly the same problem.

a.k.a:

LSMs need a scalable way to know if they're enabled that makes 
everyone happy ( especially Linus ;) ).

Regads to all,

-- 

"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 BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Ahmed S. Darwish]

On Wed, Mar 05, 2008 at 02:44:45PM +0200, Ahmed S. Darwish wrote:
> On Tue, Mar 04, 2008 at 09:45:04AM -0800, Casey Schaufler wrote:
> > > From: Linus Torvalds <torvalds@linux-foundation.org>
...
> > > 
> > > What is the oops? Why does it happen?
> ...
> > 
> > One solution would be to tighten the smackfs code so that it
> > handles the uninitialized LSM case properly.
> > 
> 
> IMHO no smackfs code should ever execute if smack isn't loaded.
> 
> This means catching it from the very fist step where it registers
> itself in init_smk_fs instead of doing several if(we're enabled) cases
> in the code path.
> 
> The solution should be a _general_ solution, _not_ a SMACK one cause 
> SELinux sufferes from exactly the same problem.
> 

Not to be misunderstood here, I'm used to writing SMACK in capitals.
I ,ofcourse, don't mean shouting (never will).

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 BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Ahmed S. Darwish]

Hi Linus,

[Adding SELinux devs to CC list, please follow to the SELinux point.]

On Tue, Mar 04, 2008 at 09:21:19AM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 4 Mar 2008, Ahmed S. Darwish wrote:
> > 
> > Smackfs initialization without an enabled Smack leads to
> > an early Oops that renders the system unusable.
> 
> I really think this is bogus. Global enables like this are just wrong, and 
> a sign that something else bad is going on.
> 
> What is the oops? Why does it happen?
> 

The problem occurs when Smack is built-in the kernel but not chosen
to register itself on boot. Smack was not chosen on boot cause either
security=AnotherLSM or security=NonExistentLSM.

In all cases, init_smk_fs() ,which registers smackfs, got called
cause it's an __initcall(init_smack_fs). 
This include the cases where smack __was not__ chosen on boot.

Making smackfs mountable when Smack is not registered leads to:

1- an Oops by dereferncing the NULL security pointer: current->security (*)

2- Smackfs code got executed though naturally all the code assumes
   that smack is already registered with the security system leading
   to several problems.

3- The bogus idea of having a subsystem interface available when the
   subsystem itself is not available!

So the global is used in init_smk_fs to not register smackfs if
Smack wasn't enabled on boot.

---- SELinux:

I think the SELinux folks faced the same problem too. In my first 
local iteration of the security= parameter patch, I forgot to set 
`selinux_disable = 1' if SELinux wasn't chosen on boot.

This led to dozen of SELinux Udev events and also led to selinuxfs 
being available even though SELinux hooks _weren't_ registered.

Regards,

(*) 
    Could not save the oops cause it occured too early, but
    it was like this:

    __init_call
    init_smk_fs(void)
    smk_unlbl_ambient(NULL)
    /* 
     * Here: current->security = NULL, cause SMACK initial setup
     * was not executed.
     */
    smack_to_secid(current->security) 
    strncmp(.., current->security, ..) 
    
-- 

"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 BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Linus Torvalds]

On Tue, 4 Mar 2008, Ahmed S. Darwish wrote:
> 
> Smackfs initialization without an enabled Smack leads to
> an early Oops that renders the system unusable.

I really think this is bogus. Global enables like this are just wrong, and 
a sign that something else bad is going on.

What is the oops? Why does it happen?

		Linus

Re: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Casey Schaufler]

----- Original Message ----
> From: Ahmed S. Darwish <darwish.07@gmail.com>
> To: Casey Schaufler <casey@schaufler-ca.com>
> Cc: LKML <linux-kernel@vger.kernel.org>; Linus <torvalds@linux-foundation.org>
> Sent: Tuesday, March 4, 2008 5:10:55 AM
> Subject: [PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded
> 
> Hi all,
> 
> Smackfs initialization without an enabled Smack leads to
> an early Oops that renders the system unusable.
> 
> Introduce a global smack_enabled variable that will be used
> to make sure that no smack components will be registered 
> (ala smackfs) if we are not already enabled.
> 
> Signed-off-by: Ahmed S. Darwish
Acked-by: Casey Schaufler casey@schaufler-ca.com

> ---
> 
> The Oops is triggered by the security= patch that will be sent
> soon.
> 
> I can't imagine an SELinux guru finding /smackfs instead of 
> his usual /selinuxfs when he hits a tab completion after 's' ;).
> As a bonus, this patch will handle that case too.
> 
> smack.h | 9 +++++++++
> smack_lsm.c | 8 ++++++++
> smackfs.c | 3 +++
> 
> 3 files changed, 20 insertions(+)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a21a0e9..17c55ad 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -18,6 +18,15 @@
> #include 
> 
> /*
> + * We must not bother the rest of the kernel by exporting our
> + * own stuff if we are not already enabled. We may not be loaded
> + * if another or no LSM was chosen on boot.
> + * Smackfs is currently the only exported component, but this
> + * may change in the future.
> + */
> +extern int smack_enabled;
> +
> +/*
> * Why 23? CIPSO is constrained to 30, so a 32 byte buffer is
> * bigger than can be used, and 24 is the next lower multiple
> * of 8, and there are too many issues if there isn't space set
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 770eb06..6fe7869 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -36,6 +36,8 @@
> #define SOCKFS_MAGIC 0x534F434B
> #define TMPFS_MAGIC 0x01021994
> 
> +int smack_enabled;
> +
> /**
> * smk_fetch - Fetch the smack label from a file.
> * @ip: a pointer to the inode
> @@ -2589,6 +2591,12 @@ static __init int smack_init(void)
> if (register_security(&smack_ops))
> panic("smack: Unable to register with kernel.\n");
> 
> + /*
> + * Notify other Smack components that it's now safe to
> + * to register themselves.
> + */
> + smack_enabled = 1;
> +
> return 0;
> }
> 
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 358c92c..e1687c0 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -992,6 +992,9 @@ static int __init init_smk_fs(void)
> {
> int err;
> 
> + if (!smack_enabled)
> + return 0;
> +
> err = register_filesystem(&smk_fs_type);
> if (!err) {
> smackfs_mount = kern_mount(&smk_fs_type);
> 
> -- 
> 
> "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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at 
> http://www.tux.org/lkml/


Casey Schaufler
casey@schaufler-ca.com 

[PATCH BUGFIX -rc3] Smack: Don't register smackfs if we're not loaded

[Ahmed S. Darwish]

Hi all,

Smackfs initialization without an enabled Smack leads to
an early Oops that renders the system unusable.

Introduce a global smack_enabled variable that will be used
to make sure that no smack components will be registered 
(ala smackfs) if we are not already enabled.

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

The Oops is triggered by the security= patch that will be sent
soon.

I can't imagine an SELinux guru finding /smackfs instead of 
his usual /selinuxfs when he hits a tab completion after 's' ;).
As a bonus, this patch will handle that case too.

 smack.h     |    9 +++++++++
 smack_lsm.c |    8 ++++++++
 smackfs.c   |    3 +++

 3 files changed, 20 insertions(+)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index a21a0e9..17c55ad 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -18,6 +18,15 @@
 #include <net/netlabel.h>
 
 /*
+ * We must not bother the rest of the kernel by exporting our
+ * own stuff if we are not already enabled. We may not be loaded
+ * if another or no LSM was chosen on boot.
+ * Smackfs is currently the only exported component, but this
+ * may change in the future.
+ */
+extern int smack_enabled;
+
+/*
  * Why 23? CIPSO is constrained to 30, so a 32 byte buffer is
  * bigger than can be used, and 24 is the next lower multiple
  * of 8, and there are too many issues if there isn't space set
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 770eb06..6fe7869 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -36,6 +36,8 @@
 #define SOCKFS_MAGIC		0x534F434B
 #define TMPFS_MAGIC		0x01021994
 
+int smack_enabled;
+
 /**
  * smk_fetch - Fetch the smack label from a file.
  * @ip: a pointer to the inode
@@ -2589,6 +2591,12 @@ static __init int smack_init(void)
 	if (register_security(&smack_ops))
 		panic("smack: Unable to register with kernel.\n");
 
+	/*
+	 * Notify other Smack components that it's now safe to
+	 * to register themselves.
+	 */
+	smack_enabled = 1;
+
 	return 0;
 }
 
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 358c92c..e1687c0 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -992,6 +992,9 @@ static int __init init_smk_fs(void)
 {
 	int err;
 
+	if (!smack_enabled)
+		return 0;
+
 	err = register_filesystem(&smk_fs_type);
 	if (!err) {
 		smackfs_mount = kern_mount(&smk_fs_type);

-- 

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

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