[PATCH] security_ops locking

jmorrisat_private

The patch below implements locking of for the security module registration 
interfaces, covering writing & testing of the security_ops pointer.

It is possible for races to exist when there are multiple callers of any 
of register_security(),  unregister_security(), mod_reg_security() and 
mod_unreg_security() during testing and manipulation of the security_ops 
pointer.  For example, you could have two callers succeeding in a call to 
register_security(), with only one succeeding in actually having its 
security ops pointer recorded with the framework (the other would be 
overwritten),

Typically this will not be encountered, as module load/unload occurs under 
locks, but it is possible for these interfaces to be called from other 
contexts.  I'm not sure if this is too paranoid, but it should at least be 
safe and correct under all conditions.

David raised an issue regarding atomic pointer assignments -- if this is 
indeed a problem on any arch, the new spinlock will impose appropriate 
memory ordering for security_ops pointer assignments.

Comments welcome.

- James
-- 
James Morris
<jmorrisat_private>

diff -burN -X dontdiff lsm-2.5/security/security.c lsm-2.5.w2/security/security.c

--- lsm-2.5/security/security.c	Tue Jul 23 11:24:17 2002
+++ lsm-2.5.w2/security/security.c	Thu Jul 25 02:12:47 2002
@@ -16,13 +16,18 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/spinlock.h>
 #include <linux/security.h>
 
 #define SECURITY_SCAFFOLD_VERSION	"1.0.0"
 
 extern struct security_operations dummy_security_ops;	/* lives in dummy.c */
 
+/* The security_ops pointer should only be set or tested while security_ops_lock
+ * is held (except during scaffolding startup).
+ */
 struct security_operations *security_ops;	/* Initialized to NULL */
+static spinlock_t security_ops_lock = SPIN_LOCK_UNLOCKED;
 
 /* This macro checks that all pointers in a struct are non-NULL.  It 
  * can be fooled by struct padding for object tile alignment and when
@@ -106,13 +111,18 @@
  */
 int register_security (struct security_operations *ops)
 {
+	unsigned long flags;
 
 	if (verify (ops)) {
 		printk (KERN_INFO __FUNCTION__ " could not verify "
 			"security_operations structure.\n");
 		return -EINVAL;
 	}
+	
+	spin_lock_irqsave (&security_ops_lock, flags);
+	
 	if (security_ops != &dummy_security_ops) {
+		spin_unlock_irqrestore (&security_ops_lock, flags);
 		printk (KERN_INFO "There is already a security "
 			"framework initialized, " __FUNCTION__ " failed.\n");
 		return -EINVAL;
@@ -120,6 +130,8 @@
 
 	security_ops = ops;
 
+	spin_unlock_irqrestore (&security_ops_lock, flags);
+	
 	return 0;
 }
 
@@ -136,7 +148,12 @@
  */
 int unregister_security (struct security_operations *ops)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave (&security_ops_lock, flags);
+	
 	if (ops != security_ops) {
+		spin_unlock_irqrestore (&security_ops_lock, flags);
 		printk (KERN_INFO __FUNCTION__ ": trying to unregister "
 			"a security_opts structure that is not "
 			"registered, failing.\n");
@@ -145,6 +162,8 @@
 
 	security_ops = &dummy_security_ops;
 
+	spin_unlock_irqrestore (&security_ops_lock, flags);
+	
 	return 0;
 }
 
@@ -162,18 +181,25 @@
  */
 int mod_reg_security (const char *name, struct security_operations *ops)
 {
+        unsigned long flags;
+        
 	if (verify (ops)) {
 		printk (KERN_INFO __FUNCTION__ " could not verify "
 			"security operations.\n");
 		return -EINVAL;
 	}
 
+	spin_lock_irqsave (&security_ops_lock, flags);
+	
 	if (ops == security_ops) {
+		spin_unlock_irqrestore (&security_ops_lock, flags);
 		printk (KERN_INFO __FUNCTION__ " security operations "
 			"already registered.\n");
 		return -EINVAL;
 	}
 
+	spin_unlock_irqrestore (&security_ops_lock, flags);
+	
 	return security_ops->register_security (name, ops);
 }
 
@@ -192,12 +218,19 @@
  */
 int mod_unreg_security (const char *name, struct security_operations *ops)
 {
+	unsigned long flags;
+	
+	spin_lock_irqsave (&security_ops_lock, flags);
+	
 	if (ops == security_ops) {
+		spin_unlock_irqrestore (&security_ops_lock, flags);
 		printk (KERN_INFO __FUNCTION__ " invalid attempt to unregister "
 			" primary security ops.\n");
 		return -EINVAL;
 	}
 
+	spin_unlock_irqrestore (&security_ops_lock, flags);
+	
 	return security_ops->unregister_security (name, ops);
 }
 

_______________________________________________
linux-security-module mailing list
linux-security-moduleat_private
http://mail.wirex.com/mailman/listinfo/linux-security-module
