[PATCH 0/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

[PATCH 0/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

[Casey Schaufler]

The Smack patch and Paul Moore's netlabel API patch,
together for 2.6.24-rc1. Paul's changes are identical
to the previous posting, but it's been a while so they're
here again.

The sole intent of change has been to address locking
and/or list processing issues. Please don't hesitate to
point out any problems that you might see or suggest
alternatives where things might not be to your liking.

This version is aimed at 2.6.24, and has been tested
against 2.6.24-rc1.

Thank you again.

Re: [PATCH 0/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

[Joshua Brindle]

Casey Schaufler wrote:
> The Smack patch and Paul Moore's netlabel API patch,
> together for 2.6.24-rc1. Paul's changes are identical
> to the previous posting, but it's been a while so they're
> here again.
>
> The sole intent of change has been to address locking
> and/or list processing issues. Please don't hesitate to
> point out any problems that you might see or suggest
> alternatives where things might not be to your liking.
>
> This version is aimed at 2.6.24, and has been tested
> against 2.6.24-rc1.
>
> Thank you again.

For some reason I didn't get 2/2 so I can't make an inline comment but 
one thing I noticed while trying to build a smack kernel is that smack 
has a kconfig dependancy on NETLABEL and SECURITY_NETWORK. This is 
unfortunate because user X wanting to try out smack won't see it in the 
config until he goes and enables those things (which he wouldn't know 
about without reading the smack Kconfig). It would be nice if those were 
selects instead.

Re: [PATCH 0/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

[Joshua Brindle]

Casey Schaufler wrote:
> The Smack patch and Paul Moore's netlabel API patch,
> together for 2.6.24-rc1. Paul's changes are identical
> to the previous posting, but it's been a while so they're
> here again.
>
> The sole intent of change has been to address locking
> and/or list processing issues. Please don't hesitate to
> point out any problems that you might see or suggest
> alternatives where things might not be to your liking.
>
> This version is aimed at 2.6.24, and has been tested
> against 2.6.24-rc1.
>   
with both of these patches applied to 2.6.24-rc1 I get the following 
oops when nfsd starts:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0000013c
printing eip: c01d7e39 *pde = 00000000
Oops: 0000 [#1] SMP

Pid: 4094, comm: lockd Not tainted (2.6.24-rc1 #3)
EIP: 0060:[<c01d7e39>] EFLAGS: 00010246 CPU: 0
EIP is at smack_socket_post_create+0x46/0xd2
EAX: c19440c0 EBX: 00000000 ECX: 00000001 EDX: c168ddd8
ESI: 00000002 EDI: 00000000 EBP: 00000006 ESP: c168ddd8
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process lockd (pid: 4094, ti=c168c000 task=c1577ab0 task.ti=c168c000)
Stack: c1464c00 c01d7ac6 c19440e8 c01d4fb4 c016544c c038c660 c19440c0 
00000001
       c01d53eb 00000006 00000001 fffffff4 c0283374 00000006 00000001 
00000002
       c1944540 c168df34 c1944540 00000800 c02833b6 c168df34 c1944540 
c2039d8c
Call Trace:
 [<c01d7ac6>] smack_inode_alloc_security+0x14/0x24
 [<c01d4fb4>] security_inode_alloc+0x16/0x17
 [<c016544c>] alloc_inode+0x118/0x170
 [<c01d53eb>] security_socket_post_create+0x1f/0x23
 [<c0283374>] sock_create_lite+0x4d/0x6c
 [<c02833b6>] kernel_accept+0x23/0x5a
 [<c02d8aac>] svc_tcp_recvfrom+0xf9/0x7e7
 [<c0120be5>] run_timer_softirq+0x2f/0x154
 [<c01125df>] __update_rq_clock+0x19/0x156
 [<c012e30c>] clocksource_get_next+0x39/0x3f
 [<c012d55b>] update_wall_time+0x54b/0x6af
 [<c02e2852>] schedule+0x575/0x58f
 [<c02d87c1>] svc_udp_recvfrom+0x175/0x367
 [<c01279fc>] __rcu_process_callbacks+0xeb/0x153
 [<c02e294d>] schedule_timeout+0x13/0x8d
 [<c02d7b4d>] svc_sock_release+0xdd/0x149
 [<c02d8596>] svc_recv+0x2df/0x395
 [<c0103078>] apic_timer_interrupt+0x28/0x30
 [<c0114e8f>] default_wake_function+0x0/0x8
 [<c01c881c>] lockd+0xe3/0x1f3
 [<c0116fa2>] schedule_tail+0x18/0x52
 [<c01024f6>] ret_from_fork+0x6/0x1c
 [<c01c8739>] lockd+0x0/0x1f3
 [<c01c8739>] lockd+0x0/0x1f3
 [<c01031fb>] kernel_thread_helper+0x7/0x10
 =======================
Code: 38 c0 75 0c 64 a1 00 c0 3d c0 8b 80 c0 04 00 00 e8 31 f5 ff ff 89 
83 64 01 00 00 31 ff 83 fe 02 0f 85 88 00 00 00 8b 5b 14 89 e2 <8b> 83 
3c 01 00 00 c7 04 24 00 00 00 00 c7 44 24 04 00 00 00 00
EIP: [<c01d7e39>] smack_socket_post_create+0x46/0xd2 SS:ESP 0068:c168ddd8
BUG: unable to handle kernel NULL pointer dereference at virtual address 
0000013c
printing eip: c01d7e39 *pde = 00000000
Oops: 0000 [#2] SMP

Pid: 4095, comm: nfsd Tainted: G      D (2.6.24-rc1 #3)
EIP: 0060:[<c01d7e39>] EFLAGS: 00010246 CPU: 0
EIP is at smack_socket_post_create+0x46/0xd2
EAX: c1944240 EBX: 00000000 ECX: 00000001 EDX: c1603e00
ESI: 00000002 EDI: 00000000 EBP: 00000006 ESP: c1603e00
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process nfsd (pid: 4095, ti=c1602000 task=c1559ab0 task.ti=c1602000)
Stack: c1464c00 c01d7ac6 c1944268 c01d4fb4 c016544c c038c660 c1944240 
00000001
       c01d53eb 00000006 00000001 fffffff4 c0283374 00000006 00000001 
00000002
       c19443c0 c1603f5c c19443c0 00000800 c02833b6 c1603f5c c19443c0 
c16c9e00
Call Trace:
 [<c01d7ac6>] smack_inode_alloc_security+0x14/0x24
 [<c01d4fb4>] security_inode_alloc+0x16/0x17
 [<c016544c>] alloc_inode+0x118/0x170
 [<c01d53eb>] security_socket_post_create+0x1f/0x23
 [<c0283374>] sock_create_lite+0x4d/0x6c
 [<c02833b6>] kernel_accept+0x23/0x5a
 [<c02d8aac>] svc_tcp_recvfrom+0xf9/0x7e7
 [<c0111b7b>] __wake_up_common+0x32/0x5c
 [<c02e37c3>] _spin_lock_bh+0x8/0x18
 [<c0284761>] lock_sock_nested+0x84/0x8c
 [<c02d87c1>] svc_udp_recvfrom+0x175/0x367
 [<c02d7b4d>] svc_sock_release+0xdd/0x149
 [<c02d8596>] svc_recv+0x2df/0x395
 [<c0113210>] sched_move_task+0xa0/0xa7
 [<c0114e8f>] default_wake_function+0x0/0x8
 [<c01bb125>] nfsd+0xcc/0x27b
 [<c01bb059>] nfsd+0x0/0x27b
 [<c01031fb>] kernel_thread_helper+0x7/0x10
 =======================
Code: 38 c0 75 0c 64 a1 00 c0 3d c0 8b 80 c0 04 00 00 e8 31 f5 ff ff 89 
83 64 01 00 00 31 ff 83 fe 02 0f 85 88 00 00 00 8b 5b 14 89 e2 <8b> 83 
3c 01 00 00 c7 04 24 00 00 00 00 c7 44 24 04 00 00 00 00
EIP: [<c01d7e39>] smack_socket_post_create+0x46/0xd2 SS:ESP 0068:c1603e00

Re: [PATCH 0/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

[Casey Schaufler]

--- Joshua Brindle <method@manicmethod.com> wrote:

> Casey Schaufler wrote:
> > The Smack patch and Paul Moore's netlabel API patch,
> > together for 2.6.24-rc1. Paul's changes are identical
> > to the previous posting, but it's been a while so they're
> > here again.
> >
> > The sole intent of change has been to address locking
> > and/or list processing issues. Please don't hesitate to
> > point out any problems that you might see or suggest
> > alternatives where things might not be to your liking.
> >
> > This version is aimed at 2.6.24, and has been tested
> > against 2.6.24-rc1.
> >   
> with both of these patches applied to 2.6.24-rc1 I get the following 
> oops when nfsd starts:
> 
> BUG: unable to handle kernel NULL pointer dereference at virtual address 
> 0000013c
> printing eip: c01d7e39 *pde = 00000000
> Oops: 0000 [#1] SMP
>
> ....

Thanks for the bug report. I have just discovered that it is possible
to have a virtual disk go virtually bad under VMware. I will be
looking at this as soon as I recover.


Casey Schaufler
casey@schaufler-ca.com

[PATCH] Smackv9: Use a stateful parser for parsing Smack rules

[Ahmed S. Darwish]

Hi Casey/Al/all,

A patch that utilizes Al Viro's concerns on previous smack parser
and solves pevious parser bugs discovered by Ahmed Darwish. By now,
no problem will occur if given smack rules are fragmented over
multiple write() calls.

CC: Al Viro <viro@ftp.linux.org.uk>
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

A similar patch for parsing CIPSO rules will be sent soon.

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 88b3f7b..9b56281 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -67,6 +67,39 @@ static int smack_cipso_count;
 struct smk_list_entry *smack_list;
 
 /*
+ * Disable concurrent writing open() operations
+ */
+static struct semaphore smack_write_sem;
+
+/*
+ * States for parsing /smack/load rules
+ */
+enum load_state {
+	subject      = 0,
+	object       = 1,
+	access       = 2,
+	eol          = 3,
+};
+
+/*
+ * Represent current parsing state of /smack/load. Struct
+ * also stores data needed between an open-release session's
+ * multiple write() calls
+ */
+static struct smack_load_state {
+	enum load_state state;
+	struct  smack_rule rule;
+	int     label_len;
+	char    subject[SMK_LABELLEN];
+	char    object[SMK_LABELLEN];
+} *load_state;
+
+static inline int isblank(char c)
+{
+	return (c == ' ' || c == '\t');
+}
+
+/*
  * Seq_file read operations for /smack/load
  */
 
@@ -127,12 +160,43 @@ static struct seq_operations load_seq_ops = {
  * @inode: inode structure representing file
  * @file: "load" file pointer
  *
- * Connect our load_seq_* operations with /smack/load
- * file_operations
+ * For reading, use load_seq_* seq_file reading operations.
+ * For writing, prepare a load_state struct to parse
+ * incoming rules.
  */
 static int smk_open_load(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &load_seq_ops);
+	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
+		return seq_open(file, &load_seq_ops);
+
+	if (down_interruptible(&smack_write_sem))
+		return -ERESTARTSYS;
+
+	load_state = kzalloc(sizeof(struct smack_load_state), GFP_KERNEL);
+	if (!load_state)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * smk_release_load - release() for /smack/load
+ * @inode: inode structure representing file
+ * @file: "load" file pointer
+ *
+ * For a reading session, use the seq_file release
+ * implementation.
+ * Otherwise, we are at the end of a writing session so
+ * clean everything up.
+ */
+static int smk_release_load(struct inode *inode, struct file *file)
+{
+	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
+		return seq_release(inode, file);
+
+	kfree(load_state);
+	up(&smack_write_sem);
+	return 0;
 }
 
 /**
@@ -171,7 +235,6 @@ static void smk_set_access(struct smack_rule *srp)
 	return;
 }
 
-
 /**
  * smk_write_load - write() for /smack/load
  * @filp: file pointer, not actually used
@@ -179,19 +242,20 @@ static void smk_set_access(struct smack_rule *srp)
  * @count: bytes sent
  * @ppos: where to start
  *
- * Returns number of bytes written or error code, as appropriate
+ * Parse smack rules of format "subject object accesss". Handle
+ * defragmented rules over several write calls using the
+ * load_state structure.
  */
 static ssize_t smk_write_load(struct file *file, const char __user *buf,
 			      size_t count, loff_t *ppos)
 {
-	struct smack_rule rule;
-	ssize_t rc = count;
+	struct smack_rule *rule = &load_state->rule;
+	int *label_len = &load_state->label_len;
+	char *subjectstr = load_state->subject;
+	char *objectstr = load_state->object;
+	ssize_t rc = -EINVAL;
 	char *data = NULL;
-	char subjectstr[SMK_LABELLEN];
-	char objectstr[SMK_LABELLEN];
-	char modestr[8];
-	char *cp;
-
+	int i;
 
 	if (!capable(CAP_MAC_OVERRIDE))
 		return -EPERM;
@@ -205,7 +269,7 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 	 * 80 characters per line ought to be enough.
 	 */
 	if (count > SMACK_LIST_MAX * 80)
-		return -ENOMEM;
+		return -EFBIG;
 
 	data = kzalloc(count + 1, GFP_KERNEL);
 	if (data == NULL)
@@ -218,30 +282,74 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 
 	*(data + count) = '\0';
 
-	for (cp = data - 1; cp != NULL; cp = strchr(cp + 1, '\n')) {
-		if (*++cp == '\0')
+	for (i = 0; i < count && data[i]; i ++)
+		switch (load_state->state) {
+		case subject:
+			if (isblank(data[i])) {
+				load_state->state = object;
+				subjectstr[*label_len] = '\0';
+				*label_len = 0;
+				break;
+			}
+			if (*label_len >= SMK_MAXLEN)
+				goto out;
+			subjectstr[(*label_len) ++] = data[i];
 			break;
-		if (sscanf(cp, "%23s %23s %7s\n", subjectstr, objectstr,
-			   modestr) != 3)
+
+		case object:
+			if (isblank(data[i])) {
+				load_state->state = access;
+				objectstr[*label_len] = '\0';
+				*label_len = 0;
+				break;
+			}
+
+			if (*label_len >= SMK_MAXLEN)
+				goto out;
+			objectstr[(*label_len) ++] = data[i];
 			break;
-		rule.smk_subject = smk_import(subjectstr, 0);
-		if (rule.smk_subject == NULL)
+
+		case access:
+			if (isblank(data[i]) || data[i] == '\n') {
+				load_state->state = eol;
+				/* Let "eol" take control from current char */
+				--i;
+				break;
+			}
+
+			if (data[i] == 'r')
+				rule->smk_access |= MAY_READ;
+			else if (data[i] == 'w')
+				rule->smk_access |= MAY_WRITE;
+			else if (data[i] == 'x')
+				rule->smk_access |= MAY_EXEC;
+			else if (data[i] == 'a')
+				rule->smk_access |= MAY_APPEND;
+			else
+				goto out;
 			break;
-		rule.smk_object = smk_import(objectstr, 0);
-		if (rule.smk_object == NULL)
+
+		case eol:
+			if (isblank(data[i]))
+				break;
+
+			load_state->state = subject;
+			*label_len = 0;
+
+			rule->smk_subject = smk_import(subjectstr, 0);
+			if (rule->smk_subject == NULL)
+				goto out;
+
+			rule->smk_object = smk_import(objectstr, 0);
+			if (rule->smk_object == NULL)
+				goto out;
+
+			smk_set_access(rule);
 			break;
-		rule.smk_access = 0;
-		if (strpbrk(modestr, "rR") != NULL)
-			rule.smk_access |= MAY_READ;
-		if (strpbrk(modestr, "wW") != NULL)
-			rule.smk_access |= MAY_WRITE;
-		if (strpbrk(modestr, "xX") != NULL)
-			rule.smk_access |= MAY_EXEC;
-		if (strpbrk(modestr, "aA") != NULL)
-			rule.smk_access |= MAY_APPEND;
-		smk_set_access(&rule);
-	}
+		}
 
+	rc = count;
+out:
 	kfree(data);
 	return rc;
 }
@@ -251,7 +359,7 @@ static const struct file_operations smk_load_ops = {
 	.read		= seq_read,
 	.llseek         = seq_lseek,
 	.write		= smk_write_load,
-	.release        = seq_release
+	.release        = smk_release_load,
 };
 
 /**
@@ -921,6 +1029,7 @@ static int __init init_smk_fs(void)
 		}
 	}
 
+	sema_init(&smack_write_sem, 1);
 	smk_cipso_doi();
 
 	return err;

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

Re: [PATCH] Smackv9: Use a stateful parser for parsing Smack rules

[Jan Engelhardt]

On Nov 1 2007 17:54, Ahmed S. Darwish wrote:
>+
>+static inline int isblank(char c)
>+{
>+	return (c == ' ' || c == '\t');
>+}

Use isspace().

>+	for (i = 0; i < count && data[i]; i ++)
>...
>+			subjectstr[(*label_len) ++] = data[i];

i++ w/o space

Re: [PATCH] Smackv9: Use a stateful parser for parsing Smack rules

[Ahmed S. Darwish]

On 11/1/07, Jan Engelhardt <jengelh@computergmbh.de> wrote:
>
> On Nov 1 2007 17:54, Ahmed S. Darwish wrote:
> >+
> >+static inline int isblank(char c)
> >+{
> >+      return (c == ' ' || c == '\t');
> >+}
>
> Use isspace().
>

isspace accepts newlines and carriage-returns too which is not
accepted between elements of a one rule.

> >+      for (i = 0; i < count && data[i]; i ++)
> >...
> >+                      subjectstr[(*label_len) ++] = data[i];
>
> i++ w/o space
>

Notes Taken. Thanks for the review.

Regards,

Darwish