[PATCH] Smack: SMACK_IOCLOADACCESS

[PATCH] Smack: SMACK_IOCLOADACCESS

[Jarkko Sakkinen]

IOCTL call for /smack/load that takes access rule in
the same format as they are written into /smack/load.
Sets errno to zero if access is allowed and to EACCES
if not.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
---
 include/linux/smack.h    |   29 ++++++++
 security/smack/smack.h   |    1 +
 security/smack/smackfs.c |  173 +++++++++++++++++++++++++++++-----------------
 3 files changed, 140 insertions(+), 63 deletions(-)
 create mode 100644 include/linux/smack.h

diff --git a/include/linux/smack.h b/include/linux/smack.h
new file mode 100644
index 0000000..c574713
--- /dev/null
+++ b/include/linux/smack.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2011, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ *
+ * Author:
+ *      Jarkko Sakkinen <jarkko.sakkinen@intel.com>
+ *
+ */
+
+#ifndef _SMACK_H
+#define _SMACK_H
+
+#define SMACK_IOC_BASE 'S'
+#define SMACK_IOC_LOAD_ACCESS _IOR(SMACK_IOC_BASE, 0, const char *)
+
+#endif /* _SMACK_H */
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 2b6c6a5..82c5a30 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -189,6 +189,7 @@ struct smk_audit_info {
 	struct common_audit_data a;
 #endif
 };
+
 /*
  * These functions are in smack_lsm.c
  */
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index f934601..68168d0 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -27,6 +27,7 @@
 #include <linux/seq_file.h>
 #include <linux/ctype.h>
 #include <linux/audit.h>
+#include <linux/smack.h>
 #include "smack.h"
 
 /*
@@ -176,71 +177,19 @@ static int smk_set_access(struct smack_rule *srp, struct list_head *rule_list,
 }
 
 /**
- * smk_write_load_list - write() for any /smack/load
- * @file: file pointer, not actually used
- * @buf: where to get the data from
- * @count: bytes sent
- * @ppos: where to start - must be 0
- * @rule_list: the list of rules to write to
- * @rule_lock: lock for the rule list
- *
- * Get one smack access rule from above.
- * The format is exactly:
- *     char subject[SMK_LABELLEN]
- *     char object[SMK_LABELLEN]
- *     char access[SMK_ACCESSLEN]
- *
- * writes must be SMK_LABELLEN+SMK_LABELLEN+SMK_ACCESSLEN bytes.
+ * smk_parse_rule - parse subject, object and access type
+ * @data: string to be parsed whose size is SMK_LOADLEN
+ * @rule: parsed entities are stored in here
  */
-static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
-				size_t count, loff_t *ppos,
-				struct list_head *rule_list,
-				struct mutex *rule_lock)
+static int smk_parse_rule(const char *data, struct smack_rule *rule)
 {
-	struct smack_rule *rule;
-	char *data;
-	int rc = -EINVAL;
-
-	/*
-	 * No partial writes.
-	 * Enough data must be present.
-	 */
-	if (*ppos != 0)
-		return -EINVAL;
-	/*
-	 * Minor hack for backward compatibility
-	 */
-	if (count < (SMK_OLOADLEN) || count > SMK_LOADLEN)
-		return -EINVAL;
-
-	data = kzalloc(SMK_LOADLEN, GFP_KERNEL);
-	if (data == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(data, buf, count) != 0) {
-		rc = -EFAULT;
-		goto out;
-	}
-
-	/*
-	 * More on the minor hack for backward compatibility
-	 */
-	if (count == (SMK_OLOADLEN))
-		data[SMK_OLOADLEN] = '-';
-
-	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
-	if (rule == NULL) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
 	rule->smk_subject = smk_import(data, 0);
 	if (rule->smk_subject == NULL)
-		goto out_free_rule;
+		return -1;
 
 	rule->smk_object = smk_import(data + SMK_LABELLEN, 0);
 	if (rule->smk_object == NULL)
-		goto out_free_rule;
+		return -1;
 
 	rule->smk_access = 0;
 
@@ -252,7 +201,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
 		rule->smk_access |= MAY_READ;
 		break;
 	default:
-		goto out_free_rule;
+		return -1;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 1]) {
@@ -263,7 +212,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
 		rule->smk_access |= MAY_WRITE;
 		break;
 	default:
-		goto out_free_rule;
+		return -1;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 2]) {
@@ -274,7 +223,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
 		rule->smk_access |= MAY_EXEC;
 		break;
 	default:
-		goto out_free_rule;
+		return -1;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 3]) {
@@ -285,7 +234,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
 		rule->smk_access |= MAY_APPEND;
 		break;
 	default:
-		goto out_free_rule;
+		return -1;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 4]) {
@@ -296,9 +245,74 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
 		rule->smk_access |= MAY_TRANSMUTE;
 		break;
 	default:
-		goto out_free_rule;
+		return -1;
+	}
+
+	return 0;
+}
+
+/**
+ * smk_write_load_list - write() for any /smack/load
+ * @file: file pointer, not actually used
+ * @buf: where to get the data from
+ * @count: bytes sent
+ * @ppos: where to start - must be 0
+ * @rule_list: the list of rules to write to
+ * @rule_lock: lock for the rule list
+ *
+ * Get one smack access rule from above.
+ * The format is exactly:
+ *     char subject[SMK_LABELLEN]
+ *     char object[SMK_LABELLEN]
+ *     char access[SMK_ACCESSLEN]
+ *
+ * writes must be SMK_LABELLEN+SMK_LABELLEN+SMK_ACCESSLEN bytes.
+ */
+static ssize_t smk_write_load_list(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos,
+				struct list_head *rule_list,
+				struct mutex *rule_lock)
+{
+	struct smack_rule *rule;
+	char *data;
+	int rc = -EINVAL;
+
+	/*
+	 * No partial writes.
+	 * Enough data must be present.
+	 */
+	if (*ppos != 0)
+		return -EINVAL;
+	/*
+	 * Minor hack for backward compatibility
+	 */
+	if (count < (SMK_OLOADLEN) || count > SMK_LOADLEN)
+		return -EINVAL;
+
+	data = kzalloc(SMK_LOADLEN, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	if (copy_from_user(data, buf, count) != 0) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	/*
+	 * More on the minor hack for backward compatibility
+	 */
+	if (count == (SMK_OLOADLEN))
+		data[SMK_OLOADLEN] = '-';
+
+	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
+	if (rule == NULL) {
+		rc = -ENOMEM;
+		goto out;
 	}
 
+	if (smk_parse_rule(data, rule))
+		goto out_free_rule;
+
 	rc = count;
 	/*
 	 * smk_set_access returns true if there was already a rule
@@ -416,12 +430,45 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 					&smack_list_lock);
 }
 
+/**
+ * smk_ioctl_load - ioctl() for /smack/load
+ * @file: file pointer, not actually used
+ * @cmd: IOCTL command
+ * @arg: IOCTL command argument
+ */
+static long smk_ioctl_load(struct file *file, unsigned int cmd,
+			   unsigned long arg)
+{
+	char data[SMK_LOADLEN];
+	struct smack_rule rule;
+
+	/* Currently we have only single IOCTL command supported */
+	if (cmd != SMACK_IOC_LOAD_ACCESS)
+		return -ENOTTY;
+
+	if (!capable(CAP_MAC_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(data, (const char __user *)arg, SMK_LOADLEN))
+		return -EFAULT;
+
+	if (smk_parse_rule(data, &rule))
+		return -EINVAL;
+
+	if (smk_access(rule.smk_subject, rule.smk_object,
+		       rule.smk_access, NULL) != 0)
+		return -EACCES;
+
+	return 0;
+}
+
 static const struct file_operations smk_load_ops = {
 	.open           = smk_open_load,
 	.read		= seq_read,
 	.llseek         = seq_lseek,
 	.write		= smk_write_load,
 	.release        = seq_release,
+	.unlocked_ioctl = smk_ioctl_load,
 };
 
 /**
-- 
1.7.4.1

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Eric Paris]

On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
<jarkko.sakkinen@intel.com> wrote:
> IOCTL call for /smack/load that takes access rule in
> the same format as they are written into /smack/load.
> Sets errno to zero if access is allowed and to EACCES
> if not.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>

[SELinux maintainer here, but Casey knew to already take what I say
with a grain of salt]

I'm not telling you to do anything differently, just telling you what
SELinux does, and why we do it.  SELinux has a file in selinuxfs
called 'access.'  The file can be opened and one can write a rule into
the file.  One then calls read and gets back a structure which
contains all of the permissions information allowed for the
source/target/class.  In SELinux we calculate all of the permissions
for the tuple at once so providing all of the information at once can
make a lot of sense.  libselinux provides libraries that will cache
these decisions in the userspace program and quickly answer the same
(or similar) questions later.

http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c

Shows the userspace side of out "access" interface.  Your interface is
good in that it only takes 1 syscall and ours takes 2.  Your interface
is bad in that it is ioctl and we are told since birth that we must
hate them no matter what (not that read/write is really any
different).  It isn't the same method the only other LSM I know about
uses.  It can only every return one value (ok, I know ioctl can be
made to do anything at all)

Anyway, just food for thought....

-Eric

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Alan Cox]

> good in that it only takes 1 syscall and ours takes 2.  Your interface
> is bad in that it is ioctl and we are told since birth that we must
> hate them no matter what (not that read/write is really any
> different).  It isn't the same method the only other LSM I know about
> uses.  It can only every return one value (ok, I know ioctl can be
> made to do anything at all)

I'm all in favour of the use of brains rather than the cult of ioctl
hating. You can design bad ioctls and good ones. Also ioctl is pretty
much unique in being bidirectional, it allows a query/respose action
without having to worry about whether the respose is the one to your
query or another parallel query.

Alan

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Sakkinen, Jarkko]

On Fri, Aug 26, 2011 at 3:50 PM, Eric Paris <eparis@parisplace.org> wrote:
> On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
> <jarkko.sakkinen@intel.com> wrote:
>> IOCTL call for /smack/load that takes access rule in
>> the same format as they are written into /smack/load.
>> Sets errno to zero if access is allowed and to EACCES
>> if not.
>>
>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
>
> [SELinux maintainer here, but Casey knew to already take what I say
> with a grain of salt]
>
> I'm not telling you to do anything differently, just telling you what
> SELinux does, and why we do it.  SELinux has a file in selinuxfs
> called 'access.'  The file can be opened and one can write a rule into
> the file.  One then calls read and gets back a structure which
> contains all of the permissions information allowed for the
> source/target/class.  In SELinux we calculate all of the permissions
> for the tuple at once so providing all of the information at once can
> make a lot of sense.  libselinux provides libraries that will cache
> these decisions in the userspace program and quickly answer the same
> (or similar) questions later.
>
> http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c

Thank you for this information. One thing that concerns
me in this approach is the scenario where things serialize
to the following sequence:

- Process A does open()
- Process B does open()
- Process A does write()
- Process B does write()
- Process A does read()
- ... (sequence continues)

What's the end result?

> Shows the userspace side of out "access" interface.  Your interface is
> good in that it only takes 1 syscall and ours takes 2.  Your interface
> is bad in that it is ioctl and we are told since birth that we must
> hate them no matter what (not that read/write is really any
> different).  It isn't the same method the only other LSM I know about
> uses.  It can only every return one value (ok, I know ioctl can be
> made to do anything at all)

I'm aware of the fact that IOCTLs should be avoided
but on the other hand in this use case I see it as the
cleanest possible API that enables clean and simple
user space support for access control and has least
risk for any side-effects.

>
> Anyway, just food for thought....
>
> -Eric
>

/Jarkko

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Casey Schaufler]

On 8/26/2011 5:50 AM, Eric Paris wrote:
> On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
> <jarkko.sakkinen@intel.com> wrote:
>> IOCTL call for /smack/load that takes access rule in
>> the same format as they are written into /smack/load.
>> Sets errno to zero if access is allowed and to EACCES
>> if not.
>>
>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
> [SELinux maintainer here, but Casey knew to already take what I say
> with a grain of salt]

Indeed.

> I'm not telling you to do anything differently, just telling you what
> SELinux does, and why we do it.  SELinux has a file in selinuxfs
> called 'access.'  The file can be opened and one can write a rule into
> the file.  One then calls read and gets back a structure which
> contains all of the permissions information allowed for the
> source/target/class.

Is that really how it works? The code reads as if the buffer
passed by write gets modified in place and the writer is expected
to use that. There is nothing in the code that looks like the
response is getting set aside for a subsequent read. If it works
the way you say it does it will be subject to races up the wazoo.
If it works the way I think the code says it does it is abusive
of the write interface.

> In SELinux we calculate all of the permissions
> for the tuple at once so providing all of the information at once can
> make a lot of sense.  libselinux provides libraries that will cache
> these decisions in the userspace program and quickly answer the same
> (or similar) questions later.

I have played with similar code for Smack and it works just fine
if you're willing to ignore rule updates and schlep all the rule
lists around in user space on the off chance someone will make an
access check. Smack is intentionally kernel oriented (hence the
"k" and the end of the name) and the ioctl requires much less
user space support than the /selinux/accesss interface, should it
be a write+read interface or an abused write.

> http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c
>
> Shows the userspace side of out "access" interface.  Your interface is
> good in that it only takes 1 syscall and ours takes 2.  Your interface
> is bad in that it is ioctl and we are told since birth that we must
> hate them no matter what (not that read/write is really any
> different).  It isn't the same method the only other LSM I know about
> uses.  It can only every return one value (ok, I know ioctl can be
> made to do anything at all)

Much better to use an ioctl for a query interface than to jump
through hoops with unnatural uses of other interfaces.

And just as a note, I was working with UNIX when ioctl was
introduced and I didn't like it then. I would not be considering
an ioctl interface if it where not the correct way to solve the
problem.


> Anyway, just food for thought....
>
> -Eric
>

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Eric Paris]

On 08/26/2011 12:05 PM, Sakkinen, Jarkko wrote:
> On Fri, Aug 26, 2011 at 3:50 PM, Eric Paris <eparis@parisplace.org> wrote:
>> On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
>> <jarkko.sakkinen@intel.com> wrote:
>>> IOCTL call for /smack/load that takes access rule in
>>> the same format as they are written into /smack/load.
>>> Sets errno to zero if access is allowed and to EACCES
>>> if not.
>>>
>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
>>
>> [SELinux maintainer here, but Casey knew to already take what I say
>> with a grain of salt]
>>
>> I'm not telling you to do anything differently, just telling you what
>> SELinux does, and why we do it.  SELinux has a file in selinuxfs
>> called 'access.'  The file can be opened and one can write a rule into
>> the file.  One then calls read and gets back a structure which
>> contains all of the permissions information allowed for the
>> source/target/class.  In SELinux we calculate all of the permissions
>> for the tuple at once so providing all of the information at once can
>> make a lot of sense.  libselinux provides libraries that will cache
>> these decisions in the userspace program and quickly answer the same
>> (or similar) questions later.
>>
>> http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c
> 
> Thank you for this information. One thing that concerns
> me in this approach is the scenario where things serialize
> to the following sequence:
> 
> - Process A does open()
> - Process B does open()
> - Process A does write()
> - Process B does write()
> - Process A does read()
> - ... (sequence continues)
> 
> What's the end result?

SELinux attaches the information needed to the struct file private area
inside the kernel using the kernel provided fs/libfs.c functions
simple_transation_*.  Which means that 2 processes have no issues
interfering with each other.  A multi threaded or misbehaving
application may get EBUSY on write() if another write()/read() combo is
in progress.  Its nice that the kernel has libraries which solve this
problem for us!

I don't know SMACK internals, but if one ever wants to have SMACK
userspace object managers the ability for the interface to only be able
to return a single value might be an eventual bottleneck.

Like I said, do whatever you guys think is best, but I'm constantly
going to point out and ask for LSM similarities when possible!

-Eric

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Randy Dunlap]

On Fri, 26 Aug 2011 08:52:07 +0300 Jarkko Sakkinen wrote:

> IOCTL call for /smack/load that takes access rule in
> the same format as they are written into /smack/load.
> Sets errno to zero if access is allowed and to EACCES
> if not.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
> ---
>  include/linux/smack.h    |   29 ++++++++
>  security/smack/smack.h   |    1 +
>  security/smack/smackfs.c |  173 +++++++++++++++++++++++++++++-----------------
>  3 files changed, 140 insertions(+), 63 deletions(-)
>  create mode 100644 include/linux/smack.h
> 
> diff --git a/include/linux/smack.h b/include/linux/smack.h
> new file mode 100644
> index 0000000..c574713
> --- /dev/null
> +++ b/include/linux/smack.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2011, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
> + *
> + * Author:
> + *      Jarkko Sakkinen <jarkko.sakkinen@intel.com>
> + *
> + */
> +
> +#ifndef _SMACK_H
> +#define _SMACK_H
> +
> +#define SMACK_IOC_BASE 'S'
> +#define SMACK_IOC_LOAD_ACCESS _IOR(SMACK_IOC_BASE, 0, const char *)
> +
> +#endif /* _SMACK_H */

Please add this 'S' to Documentation/ioctl/ioctl-number.txt.
Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Stephen Smalley]

On Fri, 2011-08-26 at 09:26 -0700, Casey Schaufler wrote:
> Is that really how it works? The code reads as if the buffer
> passed by write gets modified in place and the writer is expected
> to use that. There is nothing in the code that looks like the
> response is getting set aside for a subsequent read. If it works
> the way you say it does it will be subject to races up the wazoo.
> If it works the way I think the code says it does it is abusive
> of the write interface.

It is a transaction-based IO, modeled after nfsctl, recommended to us by
viro when we first implemented selinuxfs.  Userspace writes the input
arguments to the file, the kernel computes the result and stores it in a
file-private buffer (hence unique for each open instance, not shared by
multiple openers), and userspace then reads that result back.  Not racy,
not an abuse of write(), and recommended by the vfs maintainer to us.  I
think we're safe on that one.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Stephen Smalley]

On Fri, 2011-08-26 at 14:14 +0100, Alan Cox wrote:
> > good in that it only takes 1 syscall and ours takes 2.  Your interface
> > is bad in that it is ioctl and we are told since birth that we must
> > hate them no matter what (not that read/write is really any
> > different).  It isn't the same method the only other LSM I know about
> > uses.  It can only every return one value (ok, I know ioctl can be
> > made to do anything at all)
> 
> I'm all in favour of the use of brains rather than the cult of ioctl
> hating. You can design bad ioctls and good ones. Also ioctl is pretty
> much unique in being bidirectional, it allows a query/respose action
> without having to worry about whether the respose is the one to your
> query or another parallel query.

The transaction ops achieve that property as well - the response is
stored in an open file private buffer and thus can only correspond to a
request written to that same open file instance.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Casey Schaufler]

On 8/26/2011 10:28 AM, Stephen Smalley wrote:
> On Fri, 2011-08-26 at 09:26 -0700, Casey Schaufler wrote:
>> Is that really how it works? The code reads as if the buffer
>> passed by write gets modified in place and the writer is expected
>> to use that. There is nothing in the code that looks like the
>> response is getting set aside for a subsequent read. If it works
>> the way you say it does it will be subject to races up the wazoo.
>> If it works the way I think the code says it does it is abusive
>> of the write interface.
> It is a transaction-based IO, modeled after nfsctl, recommended to us by
> viro when we first implemented selinuxfs.

OK, I get that. Looking at the code it sure as shooting ain't obvious
to someone unfamiliar with the transaction system what is going on,
and I'm taking it on faith that it's working correctly.

> Userspace writes the input
> arguments to the file, the kernel computes the result and stores it in a
> file-private buffer (hence unique for each open instance, not shared by
> multiple openers), and userspace then reads that result back.  Not racy,

Well, in the absence of forks or multithreading.

> not an abuse of write(),

I personally think it is, but given that the transaction model
is used elsewhere I don't have a viable argument against your
using it.

> and recommended by the vfs maintainer to us.  I
> think we're safe on that one.

Yes, I concur that you have followed the rules and advice given you.
That does not mean I agree that this is a good approach.

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Casey Schaufler]

On 8/26/2011 10:01 AM, Eric Paris wrote:
> On 08/26/2011 12:05 PM, Sakkinen, Jarkko wrote:
>> On Fri, Aug 26, 2011 at 3:50 PM, Eric Paris <eparis@parisplace.org> wrote:
>>> On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
>>> <jarkko.sakkinen@intel.com> wrote:
>>>> IOCTL call for /smack/load that takes access rule in
>>>> the same format as they are written into /smack/load.
>>>> Sets errno to zero if access is allowed and to EACCES
>>>> if not.
>>>>
>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
>>> [SELinux maintainer here, but Casey knew to already take what I say
>>> with a grain of salt]
>>>
>>> I'm not telling you to do anything differently, just telling you what
>>> SELinux does, and why we do it.  SELinux has a file in selinuxfs
>>> called 'access.'  The file can be opened and one can write a rule into
>>> the file.  One then calls read and gets back a structure which
>>> contains all of the permissions information allowed for the
>>> source/target/class.  In SELinux we calculate all of the permissions
>>> for the tuple at once so providing all of the information at once can
>>> make a lot of sense.  libselinux provides libraries that will cache
>>> these decisions in the userspace program and quickly answer the same
>>> (or similar) questions later.
>>>
>>> http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c
>> Thank you for this information. One thing that concerns
>> me in this approach is the scenario where things serialize
>> to the following sequence:
>>
>> - Process A does open()
>> - Process B does open()
>> - Process A does write()
>> - Process B does write()
>> - Process A does read()
>> - ... (sequence continues)
>>
>> What's the end result?
> SELinux attaches the information needed to the struct file private area
> inside the kernel using the kernel provided fs/libfs.c functions
> simple_transation_*.  Which means that 2 processes have no issues
> interfering with each other.  A multi threaded or misbehaving
> application may get EBUSY on write() if another write()/read() combo is
> in progress.  Its nice that the kernel has libraries which solve this
> problem for us!
>
> I don't know SMACK internals, but if one ever wants to have SMACK
> userspace object managers the ability for the interface to only be able
> to return a single value might be an eventual bottleneck.

Better to have a single question answered as required and with
complete accuracy than to carry around the baggage necessary to
maintain a cached duplicate of the kernel's rules and all the
bookkeeping that requires. SELinux libraries probably have to
make a system call just to determine if the caches they are
maintaining are out of date.

>
> Like I said, do whatever you guys think is best, but I'm constantly
> going to point out and ask for LSM similarities when possible!

I'm going to argue that in this case the interface puts
excessive burden on the user space code understanding a
peculiar behavior and that an ioctl is significantly more
appropriate to the task at hand. Even if there is precedence
for doing it using transaction IO the scheme smells of foul
sorcery.

> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Eric Paris]

On 08/26/2011 05:59 PM, Casey Schaufler wrote:
> On 8/26/2011 10:01 AM, Eric Paris wrote:

> Better to have a single question answered as required and with
> complete accuracy than to carry around the baggage necessary to
> maintain a cached duplicate of the kernel's rules and all the
> bookkeeping that requires. SELinux libraries probably have to
> make a system call just to determine if the caches they are
> maintaining are out of date.

Come on Casey, this is Linux.  We have choices!  I believe there are
currently 3 solutions to solving the 'out of date caches' problem.  The
first and oldest is just a syscall per access request option (I think it
does read() but don't remember).  There is an option to get a netlink
message informing you to flush your cache.  SE-XWindows found that ONE
syscall per check was WAY WAY WAY too much overhead and uses this
method.  There is also a magic selinuxfs file which a userspace process
can mmap and just read the memory location to tell if a reload occurred.
 SE-PostgreSQL uses this method because their software architecture did
not have a good way to handle a netlink socket.

The value of the userspace cache is admittedly entirely a question of
the willingness to accept the overhead of kernel lookups.

/me checks out of thread.

-Eric

Re: [PATCH] Smack: SMACK_IOCLOADACCESS

[Sakkinen, Jarkko]

On Fri, Aug 26, 2011 at 8:01 PM, Eric Paris <eparis@redhat.com> wrote:
> On 08/26/2011 12:05 PM, Sakkinen, Jarkko wrote:
>> On Fri, Aug 26, 2011 at 3:50 PM, Eric Paris <eparis@parisplace.org> wrote:
>>> On Fri, Aug 26, 2011 at 1:52 AM, Jarkko Sakkinen
>>> <jarkko.sakkinen@intel.com> wrote:
>>>> IOCTL call for /smack/load that takes access rule in
>>>> the same format as they are written into /smack/load.
>>>> Sets errno to zero if access is allowed and to EACCES
>>>> if not.
>>>>
>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
>>>
>>> [SELinux maintainer here, but Casey knew to already take what I say
>>> with a grain of salt]
>>>
>>> I'm not telling you to do anything differently, just telling you what
>>> SELinux does, and why we do it.  SELinux has a file in selinuxfs
>>> called 'access.'  The file can be opened and one can write a rule into
>>> the file.  One then calls read and gets back a structure which
>>> contains all of the permissions information allowed for the
>>> source/target/class.  In SELinux we calculate all of the permissions
>>> for the tuple at once so providing all of the information at once can
>>> make a lot of sense.  libselinux provides libraries that will cache
>>> these decisions in the userspace program and quickly answer the same
>>> (or similar) questions later.
>>>
>>> http://userspace.selinuxproject.org/trac/browser/libselinux/src/compute_av.c
>>
>> Thank you for this information. One thing that concerns
>> me in this approach is the scenario where things serialize
>> to the following sequence:
>>
>> - Process A does open()
>> - Process B does open()
>> - Process A does write()
>> - Process B does write()
>> - Process A does read()
>> - ... (sequence continues)
>>
>> What's the end result?
>
> SELinux attaches the information needed to the struct file private area
> inside the kernel using the kernel provided fs/libfs.c functions
> simple_transation_*.  Which means that 2 processes have no issues
> interfering with each other.  A multi threaded or misbehaving
> application may get EBUSY on write() if another write()/read() combo is
> in progress.  Its nice that the kernel has libraries which solve this
> problem for us!
>
> I don't know SMACK internals, but if one ever wants to have SMACK
> userspace object managers the ability for the interface to only be able
> to return a single value might be an eventual bottleneck.

You're right. his looks like another doable way to implement
it.

I do buy your argument that it would good if Smack
would use equivalent API that is used in other places
where similar scenario applies. I'm preparing a refactored
patch that uses simple_transaction API

>
> Like I said, do whatever you guys think is best, but I'm constantly
> going to point out and ask for LSM similarities when possible!
>
> -Eric
>

/Jarkko