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...
[ message continues ]

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

--

Hi Linus,
[Adding SELinux devs to CC list, please follow to the SELinux point.]
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
--

Hi!,
[

    Our usual changelog:
    - Fold the SMACK bugfix here cause now it depends on the

      new introduced security_ symbol.

    - Do not register smackfs if Smack was not loaded

    - Do not use a global to check if Smack was enabled, use

      security_module_enable(ops) instead.

    - Export smack_ops security operations to the rest of

      SMACK code to satisfy above point.

    - Remove James ACK cause patch semantics changed (could you

      please reACK ?).

]
-->
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.
Do not let SMACK register smackfs if it was not chosen on

boot. Smackfs assumes that smack hooks are registered and

the initial task security setup (swapper->security) is done.
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>

Reported-by: Alexey Dobriyan <adobriyan@sw.ru>

---
 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.h              |    2 +

 security/smack/smack_lsm.c          |    7 ++++-

 security/smack/smackfs.c            |    9 ++++++-
 8 files changed, 87 insertions(+), 4 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt

index 9a5b665..64efbdc 100644

--- a/Documentation/kernel-parameters.txt

+++ b/Documentation/kernel-parameters.txt

@@ -374,6 +374,...
[ message continues ]

If the problem with initializing smackfs is because the

locks aren't initialized why not leave the lock initializations

in smack_init, and have them done before the check to see if the

smack LSM is going to get used? Really, we're only talking

about the case where a kernel is configured for testing or

development purposes, and the lock initialization can't
Casey Schaufler

casey@schaufler-ca.com

--

Hi Casey,
Beside the locking initialization issue, there's the current->security

issue. smackfs init code code access current->security in

smk_unlbl_ambient().
As you know current->security may equal Null (Oops), or point to

another LSM structure that preceeded us in registration. 
The locking argument can't be applied here since we may override

the other LSM tsk->security pointer this time.
Ofcourse all of the above points can be handleded by various

if(current->security) checks + rechecking the read/write methods

of each smackfs inode, but below only two lines will fix the

problem from its roots ;):
+	if (!security_module_enable(&smack_ops))

+		return 0;
Is there a problem in the current approach that I'm not aware of ?
You have your veto in this issue at the end ;)
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
--

No, I made the common mistake of replying before I'd read all the

day's threads and didn't have all the information that you did.

Current LSM usage is really unfriendly to multiple "modules". I

don't see any problems with your current approach now that I've
I suppose, although that would be really stoopid when you're

doing all the hard work. Thank you.
Casey Schaufler

casey@schaufler-ca.com

--