[PATCH 1/1 V2] mqueue: Implment generic xattr support

[PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

This patch adds support for generic extended attributes within the
POSIX message queues filesystem and setting them by consulting the LSM.
This is needed so that the security.selinux extended attribute can be
set via a SELinux named type transition on file inodes created within
the filesystem. The implementation and LSM call back function are based
off tmpfs/shmem.

Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
---
 ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 0b13ace..512a546 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -35,6 +35,7 @@
 #include <linux/ipc_namespace.h>
 #include <linux/user_namespace.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
 
 #include <net/sock.h>
 #include "util.h"
@@ -70,6 +71,7 @@ struct mqueue_inode_info {
 	struct rb_root msg_tree;
 	struct posix_msg_tree_node *node_cache;
 	struct mq_attr attr;
+	struct simple_xattrs xattrs;	/* list of xattrs */
 
 	struct sigevent notify;
 	struct pid *notify_owner;
@@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 			info->attr.mq_maxmsg = attr->mq_maxmsg;
 			info->attr.mq_msgsize = attr->mq_msgsize;
 		}
+		simple_xattrs_init(&info->xattrs);
 		/*
 		 * We used to allocate a static array of pointers and account
 		 * the size of that array as well as one msg_msg struct per
@@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
 		put_ipc_ns(ipc_ns);
 }
 
+/*
+ * Callback for security_inode_init_security() for acquiring xattrs.
+ */
+static int mqueue_initxattrs(struct inode *inode,
+			    const struct xattr *xattr_array,
+			    void *fs_info)
+{
+	struct mqueue_inode_info *info = MQUEUE_I(inode);
+	const struct xattr *xattr;
+	struct simple_xattr *new_xattr;
+	size_t len;
+
+	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+		new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
+		if (!new_xattr)
+			return -ENOMEM;
+		len = strlen(xattr->name) + 1;
+		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
+					  GFP_KERNEL);
+		if (!new_xattr->name) {
+			kfree(new_xattr);
+			return -ENOMEM;
+		}
+
+		memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
+		       XATTR_SECURITY_PREFIX_LEN);
+		memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
+		       xattr->name, len);
+
+		simple_xattr_list_add(&info->xattrs, new_xattr);
+	}
+
+	return 0;
+}
+
 static int mqueue_create(struct inode *dir, struct dentry *dentry,
 				umode_t mode, bool excl)
 {
@@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
 		ipc_ns->mq_queues_count--;
 		goto out_unlock;
 	}
+	error = security_inode_init_security(inode, dir,
+					     &dentry->d_name,
+					     mqueue_initxattrs, NULL);
+	if (error && error != -EOPNOTSUPP) {
+		spin_lock(&mq_lock);
+		ipc_ns->mq_queues_count--;
+		goto out_unlock;
+	}
 
 	put_ipc_ns(ipc_ns);
 	dir->i_size += DIRENT_SIZE;
-- 
1.9.1

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[James Morris]

On Mon, 7 Nov 2016, David Graziano wrote:

> This patch adds support for generic extended attributes within the
> POSIX message queues filesystem and setting them by consulting the LSM.
> This is needed so that the security.selinux extended attribute can be
> set via a SELinux named type transition on file inodes created within
> the filesystem. The implementation and LSM call back function are based
> off tmpfs/shmem.
> 
> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>


Acked-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Paul Moore]

On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
<david.graziano@rockwellcollins.com> wrote:
> This patch adds support for generic extended attributes within the
> POSIX message queues filesystem and setting them by consulting the LSM.
> This is needed so that the security.selinux extended attribute can be
> set via a SELinux named type transition on file inodes created within
> the filesystem. The implementation and LSM call back function are based
> off tmpfs/shmem.
>
> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
> ---
>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)

Hi David,

At first glance this looks reasonable to me, I just have a two
questions/comments:

* Can you explain your current need for this functionality?  For
example, what are you trying to do that is made easier by allowing
greater message queue labeling flexibility?  This helps put things in
context and helps people review and comment on your patch.

* How have you tested this?  While this patch is not SELinux specific,
I think adding a test to the selinux-testsuite[1] would be worthwhile.
The other LSM maintainers may suggest something similar if they have
an established public testsuite.

[1] https://github.com/SELinuxProject/selinux-testsuite

> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 0b13ace..512a546 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -35,6 +35,7 @@
>  #include <linux/ipc_namespace.h>
>  #include <linux/user_namespace.h>
>  #include <linux/slab.h>
> +#include <linux/xattr.h>
>
>  #include <net/sock.h>
>  #include "util.h"
> @@ -70,6 +71,7 @@ struct mqueue_inode_info {
>         struct rb_root msg_tree;
>         struct posix_msg_tree_node *node_cache;
>         struct mq_attr attr;
> +       struct simple_xattrs xattrs;    /* list of xattrs */
>
>         struct sigevent notify;
>         struct pid *notify_owner;
> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>                         info->attr.mq_maxmsg = attr->mq_maxmsg;
>                         info->attr.mq_msgsize = attr->mq_msgsize;
>                 }
> +               simple_xattrs_init(&info->xattrs);
>                 /*
>                  * We used to allocate a static array of pointers and account
>                  * the size of that array as well as one msg_msg struct per
> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
>                 put_ipc_ns(ipc_ns);
>  }
>
> +/*
> + * Callback for security_inode_init_security() for acquiring xattrs.
> + */
> +static int mqueue_initxattrs(struct inode *inode,
> +                           const struct xattr *xattr_array,
> +                           void *fs_info)
> +{
> +       struct mqueue_inode_info *info = MQUEUE_I(inode);
> +       const struct xattr *xattr;
> +       struct simple_xattr *new_xattr;
> +       size_t len;
> +
> +       for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> +               new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
> +               if (!new_xattr)
> +                       return -ENOMEM;
> +               len = strlen(xattr->name) + 1;
> +               new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
> +                                         GFP_KERNEL);
> +               if (!new_xattr->name) {
> +                       kfree(new_xattr);
> +                       return -ENOMEM;
> +               }
> +
> +               memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
> +                      XATTR_SECURITY_PREFIX_LEN);
> +               memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
> +                      xattr->name, len);
> +
> +               simple_xattr_list_add(&info->xattrs, new_xattr);
> +       }
> +
> +       return 0;
> +}
> +
>  static int mqueue_create(struct inode *dir, struct dentry *dentry,
>                                 umode_t mode, bool excl)
>  {
> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
>                 ipc_ns->mq_queues_count--;
>                 goto out_unlock;
>         }
> +       error = security_inode_init_security(inode, dir,
> +                                            &dentry->d_name,
> +                                            mqueue_initxattrs, NULL);
> +       if (error && error != -EOPNOTSUPP) {
> +               spin_lock(&mq_lock);
> +               ipc_ns->mq_queues_count--;
> +               goto out_unlock;
> +       }
>
>         put_ipc_ns(ipc_ns);
>         dir->i_size += DIRENT_SIZE;
> --
> 1.9.1
>
> --
> 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

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
> <david.graziano@rockwellcollins.com> wrote:
>> This patch adds support for generic extended attributes within the
>> POSIX message queues filesystem and setting them by consulting the LSM.
>> This is needed so that the security.selinux extended attribute can be
>> set via a SELinux named type transition on file inodes created within
>> the filesystem. The implementation and LSM call back function are based
>> off tmpfs/shmem.
>>
>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>> ---
>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>
> Hi David,
>
> At first glance this looks reasonable to me, I just have a two
> questions/comments:
>
> * Can you explain your current need for this functionality?  For
> example, what are you trying to do that is made easier by allowing
> greater message queue labeling flexibility?  This helps put things in
> context and helps people review and comment on your patch.
>
> * How have you tested this?  While this patch is not SELinux specific,
> I think adding a test to the selinux-testsuite[1] would be worthwhile.
> The other LSM maintainers may suggest something similar if they have
> an established public testsuite.
>
> [1] https://github.com/SELinuxProject/selinux-testsuite

Hi Paul,

I needed to write a selinux policy for a set of custom applications that use
POSIX message queues for their IPC. The queues are created by one
application and we needed a way for selinux to enforce which of the other
apps are able to read/write to each individual queue. Uniquely labeling them
based on the app that created them and the file name seemed to be our best
solution since it’s an embedded system and we don’t have restorecond to
handle any relabeling.


To test this patch I used both a selinux enabled, buildroot based qemu target
with a customized selinux policy and test C app to create the mqueues. I also
tested with our real apps and selinux policy on our target hardware. I can
certainly look at adding a test to the selinux-testsuite if that would
be helpful.

Thanks,
David

>
>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 0b13ace..512a546 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/ipc_namespace.h>
>>  #include <linux/user_namespace.h>
>>  #include <linux/slab.h>
>> +#include <linux/xattr.h>
>>
>>  #include <net/sock.h>
>>  #include "util.h"
>> @@ -70,6 +71,7 @@ struct mqueue_inode_info {
>>         struct rb_root msg_tree;
>>         struct posix_msg_tree_node *node_cache;
>>         struct mq_attr attr;
>> +       struct simple_xattrs xattrs;    /* list of xattrs */
>>
>>         struct sigevent notify;
>>         struct pid *notify_owner;
>> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>>                         info->attr.mq_maxmsg = attr->mq_maxmsg;
>>                         info->attr.mq_msgsize = attr->mq_msgsize;
>>                 }
>> +               simple_xattrs_init(&info->xattrs);
>>                 /*
>>                  * We used to allocate a static array of pointers and account
>>                  * the size of that array as well as one msg_msg struct per
>> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
>>                 put_ipc_ns(ipc_ns);
>>  }
>>
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +static int mqueue_initxattrs(struct inode *inode,
>> +                           const struct xattr *xattr_array,
>> +                           void *fs_info)
>> +{
>> +       struct mqueue_inode_info *info = MQUEUE_I(inode);
>> +       const struct xattr *xattr;
>> +       struct simple_xattr *new_xattr;
>> +       size_t len;
>> +
>> +       for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> +               new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> +               if (!new_xattr)
>> +                       return -ENOMEM;
>> +               len = strlen(xattr->name) + 1;
>> +               new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>> +                                         GFP_KERNEL);
>> +               if (!new_xattr->name) {
>> +                       kfree(new_xattr);
>> +                       return -ENOMEM;
>> +               }
>> +
>> +               memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
>> +                      XATTR_SECURITY_PREFIX_LEN);
>> +               memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
>> +                      xattr->name, len);
>> +
>> +               simple_xattr_list_add(&info->xattrs, new_xattr);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>                                 umode_t mode, bool excl)
>>  {
>> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>                 ipc_ns->mq_queues_count--;
>>                 goto out_unlock;
>>         }
>> +       error = security_inode_init_security(inode, dir,
>> +                                            &dentry->d_name,
>> +                                            mqueue_initxattrs, NULL);
>> +       if (error && error != -EOPNOTSUPP) {
>> +               spin_lock(&mq_lock);
>> +               ipc_ns->mq_queues_count--;
>> +               goto out_unlock;
>> +       }
>>
>>         put_ipc_ns(ipc_ns);
>>         dir->i_size += DIRENT_SIZE;
>> --
>> 1.9.1
>>
>> --
>> 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
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Christoph Hellwig]

> +/*
> + * Callback for security_inode_init_security() for acquiring xattrs.
> + */
> +static int mqueue_initxattrs(struct inode *inode,
> +			    const struct xattr *xattr_array,
> +			    void *fs_info)
> +{
> +	struct mqueue_inode_info *info = MQUEUE_I(inode);
> +	const struct xattr *xattr;
> +	struct simple_xattr *new_xattr;
> +	size_t len;
> +
> +	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> +		new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
> +		if (!new_xattr)
> +			return -ENOMEM;
> +		len = strlen(xattr->name) + 1;
> +		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
> +					  GFP_KERNEL);
> +		if (!new_xattr->name) {
> +			kfree(new_xattr);
> +			return -ENOMEM;
> +		}
> +
> +		memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
> +		       XATTR_SECURITY_PREFIX_LEN);
> +		memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
> +		       xattr->name, len);
> +
> +		simple_xattr_list_add(&info->xattrs, new_xattr);
> +	}
> +
> +	return 0;
> +}

This is a 1:1 copy of the shmem code, we rally should consolidate it
into a single place first, as people will want it for whatever virtual
fs they care about sooner or later.

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Paul Moore]

On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
<david.graziano@rockwellcollins.com> wrote:
> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>> <david.graziano@rockwellcollins.com> wrote:
>>> This patch adds support for generic extended attributes within the
>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>> This is needed so that the security.selinux extended attribute can be
>>> set via a SELinux named type transition on file inodes created within
>>> the filesystem. The implementation and LSM call back function are based
>>> off tmpfs/shmem.
>>>
>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>> ---
>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>
>> Hi David,
>>
>> At first glance this looks reasonable to me, I just have a two
>> questions/comments:
>>
>> * Can you explain your current need for this functionality?  For
>> example, what are you trying to do that is made easier by allowing
>> greater message queue labeling flexibility?  This helps put things in
>> context and helps people review and comment on your patch.
>>
>> * How have you tested this?  While this patch is not SELinux specific,
>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>> The other LSM maintainers may suggest something similar if they have
>> an established public testsuite.
>>
>> [1] https://github.com/SELinuxProject/selinux-testsuite
>
> Hi Paul,
>
> I needed to write a selinux policy for a set of custom applications that use
> POSIX message queues for their IPC. The queues are created by one
> application and we needed a way for selinux to enforce which of the other
> apps are able to read/write to each individual queue. Uniquely labeling them
> based on the app that created them and the file name seemed to be our best
> solution since it’s an embedded system and we don’t have restorecond to
> handle any relabeling.

In the future putting things like the above in the patch description
can be helpful.  In other words, instead of simply saying this allows
you to better control the labels assigned to message queues, you could
expand upon it by saying that this patch allows you to better control
which applications have access to a given queue.  Yes, I realize that
is implied by better control over the labels, but being explicit is
rarely a bad thing when it comes to patch descriptions.

I've never rejected a patch for a description that was too lengthy,
but I have rejected patches that need better descriptions ;)

> To test this patch I used both a selinux enabled, buildroot based qemu target
> with a customized selinux policy and test C app to create the mqueues. I also
> tested with our real apps and selinux policy on our target hardware. I can
> certainly look at adding a test to the selinux-testsuite if that would
> be helpful.

Please do.  I've been requiring tests for all new SELinux
functionality lately; this isn't strictly a SELinux patch but I think
it is a good practice regardless.

>>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>>> index 0b13ace..512a546 100644
>>> --- a/ipc/mqueue.c
>>> +++ b/ipc/mqueue.c
>>> @@ -35,6 +35,7 @@
>>>  #include <linux/ipc_namespace.h>
>>>  #include <linux/user_namespace.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/xattr.h>
>>>
>>>  #include <net/sock.h>
>>>  #include "util.h"
>>> @@ -70,6 +71,7 @@ struct mqueue_inode_info {
>>>         struct rb_root msg_tree;
>>>         struct posix_msg_tree_node *node_cache;
>>>         struct mq_attr attr;
>>> +       struct simple_xattrs xattrs;    /* list of xattrs */
>>>
>>>         struct sigevent notify;
>>>         struct pid *notify_owner;
>>> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>>>                         info->attr.mq_maxmsg = attr->mq_maxmsg;
>>>                         info->attr.mq_msgsize = attr->mq_msgsize;
>>>                 }
>>> +               simple_xattrs_init(&info->xattrs);
>>>                 /*
>>>                  * We used to allocate a static array of pointers and account
>>>                  * the size of that array as well as one msg_msg struct per
>>> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
>>>                 put_ipc_ns(ipc_ns);
>>>  }
>>>
>>> +/*
>>> + * Callback for security_inode_init_security() for acquiring xattrs.
>>> + */
>>> +static int mqueue_initxattrs(struct inode *inode,
>>> +                           const struct xattr *xattr_array,
>>> +                           void *fs_info)
>>> +{
>>> +       struct mqueue_inode_info *info = MQUEUE_I(inode);
>>> +       const struct xattr *xattr;
>>> +       struct simple_xattr *new_xattr;
>>> +       size_t len;
>>> +
>>> +       for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>>> +               new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>>> +               if (!new_xattr)
>>> +                       return -ENOMEM;
>>> +               len = strlen(xattr->name) + 1;
>>> +               new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>>> +                                         GFP_KERNEL);
>>> +               if (!new_xattr->name) {
>>> +                       kfree(new_xattr);
>>> +                       return -ENOMEM;
>>> +               }
>>> +
>>> +               memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
>>> +                      XATTR_SECURITY_PREFIX_LEN);
>>> +               memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
>>> +                      xattr->name, len);
>>> +
>>> +               simple_xattr_list_add(&info->xattrs, new_xattr);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>                                 umode_t mode, bool excl)
>>>  {
>>> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>                 ipc_ns->mq_queues_count--;
>>>                 goto out_unlock;
>>>         }
>>> +       error = security_inode_init_security(inode, dir,
>>> +                                            &dentry->d_name,
>>> +                                            mqueue_initxattrs, NULL);
>>> +       if (error && error != -EOPNOTSUPP) {
>>> +               spin_lock(&mq_lock);
>>> +               ipc_ns->mq_queues_count--;
>>> +               goto out_unlock;
>>> +       }
>>>
>>>         put_ipc_ns(ipc_ns);
>>>         dir->i_size += DIRENT_SIZE;
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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
>>
>> --
>> paul moore
>> www.paul-moore.com



-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Paul Moore]

On Wed, Nov 9, 2016 at 11:48 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +static int mqueue_initxattrs(struct inode *inode,
>> +                         const struct xattr *xattr_array,
>> +                         void *fs_info)
>> +{
>> +     struct mqueue_inode_info *info = MQUEUE_I(inode);
>> +     const struct xattr *xattr;
>> +     struct simple_xattr *new_xattr;
>> +     size_t len;
>> +
>> +     for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> +             new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> +             if (!new_xattr)
>> +                     return -ENOMEM;
>> +             len = strlen(xattr->name) + 1;
>> +             new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>> +                                       GFP_KERNEL);
>> +             if (!new_xattr->name) {
>> +                     kfree(new_xattr);
>> +                     return -ENOMEM;
>> +             }
>> +
>> +             memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
>> +                    XATTR_SECURITY_PREFIX_LEN);
>> +             memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
>> +                    xattr->name, len);
>> +
>> +             simple_xattr_list_add(&info->xattrs, new_xattr);
>> +     }
>> +
>> +     return 0;
>> +}
>
> This is a 1:1 copy of the shmem code, we rally should consolidate it
> into a single place first, as people will want it for whatever virtual
> fs they care about sooner or later.

I don't disagree, but let's keep that patch separate from the mqueue
enablement to make any future bisects cleaner.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
> <david.graziano@rockwellcollins.com> wrote:
>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>> <david.graziano@rockwellcollins.com> wrote:
>>>> This patch adds support for generic extended attributes within the
>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>> This is needed so that the security.selinux extended attribute can be
>>>> set via a SELinux named type transition on file inodes created within
>>>> the filesystem. The implementation and LSM call back function are based
>>>> off tmpfs/shmem.
>>>>
>>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>>> ---
>>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 46 insertions(+)
>>>
>>> Hi David,
>>>
>>> At first glance this looks reasonable to me, I just have a two
>>> questions/comments:
>>>
>>> * Can you explain your current need for this functionality?  For
>>> example, what are you trying to do that is made easier by allowing
>>> greater message queue labeling flexibility?  This helps put things in
>>> context and helps people review and comment on your patch.
>>>
>>> * How have you tested this?  While this patch is not SELinux specific,
>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>> The other LSM maintainers may suggest something similar if they have
>>> an established public testsuite.
>>>
>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>
>> Hi Paul,
>>
>> I needed to write a selinux policy for a set of custom applications that use
>> POSIX message queues for their IPC. The queues are created by one
>> application and we needed a way for selinux to enforce which of the other
>> apps are able to read/write to each individual queue. Uniquely labeling them
>> based on the app that created them and the file name seemed to be our best
>> solution since it’s an embedded system and we don’t have restorecond to
>> handle any relabeling.
>
> In the future putting things like the above in the patch description
> can be helpful.  In other words, instead of simply saying this allows
> you to better control the labels assigned to message queues, you could
> expand upon it by saying that this patch allows you to better control
> which applications have access to a given queue.  Yes, I realize that
> is implied by better control over the labels, but being explicit is
> rarely a bad thing when it comes to patch descriptions.
>
> I've never rejected a patch for a description that was too lengthy,
> but I have rejected patches that need better descriptions ;)
>
>> To test this patch I used both a selinux enabled, buildroot based qemu target
>> with a customized selinux policy and test C app to create the mqueues. I also
>> tested with our real apps and selinux policy on our target hardware. I can
>> certainly look at adding a test to the selinux-testsuite if that would
>> be helpful.
>
> Please do.  I've been requiring tests for all new SELinux
> functionality lately; this isn't strictly a SELinux patch but I think
> it is a good practice regardless.

Sorry for the delay. I have created a pull request within the
selinux-testsuite github
project with a set of mqueue tests.

>
>>>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>>>> index 0b13ace..512a546 100644
>>>> --- a/ipc/mqueue.c
>>>> +++ b/ipc/mqueue.c
>>>> @@ -35,6 +35,7 @@
>>>>  #include <linux/ipc_namespace.h>
>>>>  #include <linux/user_namespace.h>
>>>>  #include <linux/slab.h>
>>>> +#include <linux/xattr.h>
>>>>
>>>>  #include <net/sock.h>
>>>>  #include "util.h"
>>>> @@ -70,6 +71,7 @@ struct mqueue_inode_info {
>>>>         struct rb_root msg_tree;
>>>>         struct posix_msg_tree_node *node_cache;
>>>>         struct mq_attr attr;
>>>> +       struct simple_xattrs xattrs;    /* list of xattrs */
>>>>
>>>>         struct sigevent notify;
>>>>         struct pid *notify_owner;
>>>> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>>>>                         info->attr.mq_maxmsg = attr->mq_maxmsg;
>>>>                         info->attr.mq_msgsize = attr->mq_msgsize;
>>>>                 }
>>>> +               simple_xattrs_init(&info->xattrs);
>>>>                 /*
>>>>                  * We used to allocate a static array of pointers and account
>>>>                  * the size of that array as well as one msg_msg struct per
>>>> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
>>>>                 put_ipc_ns(ipc_ns);
>>>>  }
>>>>
>>>> +/*
>>>> + * Callback for security_inode_init_security() for acquiring xattrs.
>>>> + */
>>>> +static int mqueue_initxattrs(struct inode *inode,
>>>> +                           const struct xattr *xattr_array,
>>>> +                           void *fs_info)
>>>> +{
>>>> +       struct mqueue_inode_info *info = MQUEUE_I(inode);
>>>> +       const struct xattr *xattr;
>>>> +       struct simple_xattr *new_xattr;
>>>> +       size_t len;
>>>> +
>>>> +       for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>>>> +               new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>>>> +               if (!new_xattr)
>>>> +                       return -ENOMEM;
>>>> +               len = strlen(xattr->name) + 1;
>>>> +               new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>>>> +                                         GFP_KERNEL);
>>>> +               if (!new_xattr->name) {
>>>> +                       kfree(new_xattr);
>>>> +                       return -ENOMEM;
>>>> +               }
>>>> +
>>>> +               memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
>>>> +                      XATTR_SECURITY_PREFIX_LEN);
>>>> +               memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
>>>> +                      xattr->name, len);
>>>> +
>>>> +               simple_xattr_list_add(&info->xattrs, new_xattr);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>>                                 umode_t mode, bool excl)
>>>>  {
>>>> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>>                 ipc_ns->mq_queues_count--;
>>>>                 goto out_unlock;
>>>>         }
>>>> +       error = security_inode_init_security(inode, dir,
>>>> +                                            &dentry->d_name,
>>>> +                                            mqueue_initxattrs, NULL);
>>>> +       if (error && error != -EOPNOTSUPP) {
>>>> +               spin_lock(&mq_lock);
>>>> +               ipc_ns->mq_queues_count--;
>>>> +               goto out_unlock;
>>>> +       }
>>>>
>>>>         put_ipc_ns(ipc_ns);
>>>>         dir->i_size += DIRENT_SIZE;
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> 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
>>>
>>> --
>>> paul moore
>>> www.paul-moore.com
>
>
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Paul Moore]

On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
<david.graziano@rockwellcollins.com> wrote:
> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>> <david.graziano@rockwellcollins.com> wrote:
>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>> This patch adds support for generic extended attributes within the
>>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>>> This is needed so that the security.selinux extended attribute can be
>>>>> set via a SELinux named type transition on file inodes created within
>>>>> the filesystem. The implementation and LSM call back function are based
>>>>> off tmpfs/shmem.
>>>>>
>>>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>>>> ---
>>>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 46 insertions(+)
>>>>
>>>> Hi David,
>>>>
>>>> At first glance this looks reasonable to me, I just have a two
>>>> questions/comments:
>>>>
>>>> * Can you explain your current need for this functionality?  For
>>>> example, what are you trying to do that is made easier by allowing
>>>> greater message queue labeling flexibility?  This helps put things in
>>>> context and helps people review and comment on your patch.
>>>>
>>>> * How have you tested this?  While this patch is not SELinux specific,
>>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>>> The other LSM maintainers may suggest something similar if they have
>>>> an established public testsuite.
>>>>
>>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>>
>>> Hi Paul,
>>>
>>> I needed to write a selinux policy for a set of custom applications that use
>>> POSIX message queues for their IPC. The queues are created by one
>>> application and we needed a way for selinux to enforce which of the other
>>> apps are able to read/write to each individual queue. Uniquely labeling them
>>> based on the app that created them and the file name seemed to be our best
>>> solution since it’s an embedded system and we don’t have restorecond to
>>> handle any relabeling.
>>
>> In the future putting things like the above in the patch description
>> can be helpful.  In other words, instead of simply saying this allows
>> you to better control the labels assigned to message queues, you could
>> expand upon it by saying that this patch allows you to better control
>> which applications have access to a given queue.  Yes, I realize that
>> is implied by better control over the labels, but being explicit is
>> rarely a bad thing when it comes to patch descriptions.
>>
>> I've never rejected a patch for a description that was too lengthy,
>> but I have rejected patches that need better descriptions ;)
>>
>>> To test this patch I used both a selinux enabled, buildroot based qemu target
>>> with a customized selinux policy and test C app to create the mqueues. I also
>>> tested with our real apps and selinux policy on our target hardware. I can
>>> certainly look at adding a test to the selinux-testsuite if that would
>>> be helpful.
>>
>> Please do.  I've been requiring tests for all new SELinux
>> functionality lately; this isn't strictly a SELinux patch but I think
>> it is a good practice regardless.
>
> Sorry for the delay. I have created a pull request within the
> selinux-testsuite github
> project with a set of mqueue tests.

For anyone who is curious:

* https://github.com/SELinuxProject/selinux-testsuite/pull/10

Aside from a naming nit, the tests look good to me and I have no
problem with the kernel patch; it doesn't appear any of the other LSM
maintainers do either.  I'm happy to pull this into the SELinux tree
(for v4.11, it's a little late for v4.10 I think), but I think
Christoph made a good point about consolidation, have you had a chance
to look at that?

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
> <david.graziano@rockwellcollins.com> wrote:
>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>> <david.graziano@rockwellcollins.com> wrote:
>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>>> This patch adds support for generic extended attributes within the
>>>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>>>> This is needed so that the security.selinux extended attribute can be
>>>>>> set via a SELinux named type transition on file inodes created within
>>>>>> the filesystem. The implementation and LSM call back function are based
>>>>>> off tmpfs/shmem.
>>>>>>
>>>>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>>>>> ---
>>>>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 46 insertions(+)
>>>>>
>>>>> Hi David,
>>>>>
>>>>> At first glance this looks reasonable to me, I just have a two
>>>>> questions/comments:
>>>>>
>>>>> * Can you explain your current need for this functionality?  For
>>>>> example, what are you trying to do that is made easier by allowing
>>>>> greater message queue labeling flexibility?  This helps put things in
>>>>> context and helps people review and comment on your patch.
>>>>>
>>>>> * How have you tested this?  While this patch is not SELinux specific,
>>>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>>>> The other LSM maintainers may suggest something similar if they have
>>>>> an established public testsuite.
>>>>>
>>>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>>>
>>>> Hi Paul,
>>>>
>>>> I needed to write a selinux policy for a set of custom applications that use
>>>> POSIX message queues for their IPC. The queues are created by one
>>>> application and we needed a way for selinux to enforce which of the other
>>>> apps are able to read/write to each individual queue. Uniquely labeling them
>>>> based on the app that created them and the file name seemed to be our best
>>>> solution since it’s an embedded system and we don’t have restorecond to
>>>> handle any relabeling.
>>>
>>> In the future putting things like the above in the patch description
>>> can be helpful.  In other words, instead of simply saying this allows
>>> you to better control the labels assigned to message queues, you could
>>> expand upon it by saying that this patch allows you to better control
>>> which applications have access to a given queue.  Yes, I realize that
>>> is implied by better control over the labels, but being explicit is
>>> rarely a bad thing when it comes to patch descriptions.
>>>
>>> I've never rejected a patch for a description that was too lengthy,
>>> but I have rejected patches that need better descriptions ;)
>>>
>>>> To test this patch I used both a selinux enabled, buildroot based qemu target
>>>> with a customized selinux policy and test C app to create the mqueues. I also
>>>> tested with our real apps and selinux policy on our target hardware. I can
>>>> certainly look at adding a test to the selinux-testsuite if that would
>>>> be helpful.
>>>
>>> Please do.  I've been requiring tests for all new SELinux
>>> functionality lately; this isn't strictly a SELinux patch but I think
>>> it is a good practice regardless.
>>
>> Sorry for the delay. I have created a pull request within the
>> selinux-testsuite github
>> project with a set of mqueue tests.
>
> For anyone who is curious:
>
> * https://github.com/SELinuxProject/selinux-testsuite/pull/10
>
> Aside from a naming nit, the tests look good to me and I have no
> problem with the kernel patch; it doesn't appear any of the other LSM
> maintainers do either.  I'm happy to pull this into the SELinux tree
> (for v4.11, it's a little late for v4.10 I think), but I think
> Christoph made a good point about consolidation, have you had a chance
> to look at that?
>

I've made the update for the naming nit in the pull request.
I agree with Christoph's point but doing so is a bit outside my expertise at
this point. I would be open to suggestions as to where the function should be
consolidated and work on a second patchset with the update. Maybe in
fs/xattr.c as a simple_xattr_initxattrs function?
-David


> --
> paul moore
> www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[Paul Moore]

On Mon, Dec 5, 2016 at 11:15 AM, David Graziano
<david.graziano@rockwellcollins.com> wrote:
> On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore <paul@paul-moore.com> wrote:
>> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
>> <david.graziano@rockwellcollins.com> wrote:
>>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>>>> This patch adds support for generic extended attributes within the
>>>>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>>>>> This is needed so that the security.selinux extended attribute can be
>>>>>>> set via a SELinux named type transition on file inodes created within
>>>>>>> the filesystem. The implementation and LSM call back function are based
>>>>>>> off tmpfs/shmem.
>>>>>>>
>>>>>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>>>>>> ---
>>>>>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 46 insertions(+)
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> At first glance this looks reasonable to me, I just have a two
>>>>>> questions/comments:
>>>>>>
>>>>>> * Can you explain your current need for this functionality?  For
>>>>>> example, what are you trying to do that is made easier by allowing
>>>>>> greater message queue labeling flexibility?  This helps put things in
>>>>>> context and helps people review and comment on your patch.
>>>>>>
>>>>>> * How have you tested this?  While this patch is not SELinux specific,
>>>>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>>>>> The other LSM maintainers may suggest something similar if they have
>>>>>> an established public testsuite.
>>>>>>
>>>>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>>>>
>>>>> Hi Paul,
>>>>>
>>>>> I needed to write a selinux policy for a set of custom applications that use
>>>>> POSIX message queues for their IPC. The queues are created by one
>>>>> application and we needed a way for selinux to enforce which of the other
>>>>> apps are able to read/write to each individual queue. Uniquely labeling them
>>>>> based on the app that created them and the file name seemed to be our best
>>>>> solution since it’s an embedded system and we don’t have restorecond to
>>>>> handle any relabeling.
>>>>
>>>> In the future putting things like the above in the patch description
>>>> can be helpful.  In other words, instead of simply saying this allows
>>>> you to better control the labels assigned to message queues, you could
>>>> expand upon it by saying that this patch allows you to better control
>>>> which applications have access to a given queue.  Yes, I realize that
>>>> is implied by better control over the labels, but being explicit is
>>>> rarely a bad thing when it comes to patch descriptions.
>>>>
>>>> I've never rejected a patch for a description that was too lengthy,
>>>> but I have rejected patches that need better descriptions ;)
>>>>
>>>>> To test this patch I used both a selinux enabled, buildroot based qemu target
>>>>> with a customized selinux policy and test C app to create the mqueues. I also
>>>>> tested with our real apps and selinux policy on our target hardware. I can
>>>>> certainly look at adding a test to the selinux-testsuite if that would
>>>>> be helpful.
>>>>
>>>> Please do.  I've been requiring tests for all new SELinux
>>>> functionality lately; this isn't strictly a SELinux patch but I think
>>>> it is a good practice regardless.
>>>
>>> Sorry for the delay. I have created a pull request within the
>>> selinux-testsuite github
>>> project with a set of mqueue tests.
>>
>> For anyone who is curious:
>>
>> * https://github.com/SELinuxProject/selinux-testsuite/pull/10
>>
>> Aside from a naming nit, the tests look good to me and I have no
>> problem with the kernel patch; it doesn't appear any of the other LSM
>> maintainers do either.  I'm happy to pull this into the SELinux tree
>> (for v4.11, it's a little late for v4.10 I think), but I think
>> Christoph made a good point about consolidation, have you had a chance
>> to look at that?
>
> I've made the update for the naming nit in the pull request.

I saw that, thanks.

> I agree with Christoph's point but doing so is a bit outside my expertise at
> this point. I would be open to suggestions as to where the function should be
> consolidated and work on a second patchset with the update. Maybe in
> fs/xattr.c as a simple_xattr_initxattrs function?

That seems to make the most sense, doesn't it?  Looking at
{shmem,mqueue}_initxattrs() the only fs specific bit is the
{shmem,mqueue}_inode_info struct pointer; considering that the fs_info
parameter is currently unused in this case, you could pass a reference
to the simple_xattr struct via the fs_info parameter.

I'd CC Andreas Gruenbacher <agruenba@redhat.com> on the patch(set),
he's recently done a bunch of work around xattrs and the LSM, he may
have some additional thoughts.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 5:12 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Dec 5, 2016 at 11:15 AM, David Graziano
> <david.graziano@rockwellcollins.com> wrote:
>> On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
>>> <david.graziano@rockwellcollins.com> wrote:
>>>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>>>> <david.graziano@rockwellcollins.com> wrote:
>>>>>>>> This patch adds support for generic extended attributes within the
>>>>>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>>>>>> This is needed so that the security.selinux extended attribute can be
>>>>>>>> set via a SELinux named type transition on file inodes created within
>>>>>>>> the filesystem. The implementation and LSM call back function are based
>>>>>>>> off tmpfs/shmem.
>>>>>>>>
>>>>>>>> Signed-off-by: David Graziano <david.graziano@rockwellcollins.com>
>>>>>>>> ---
>>>>>>>>  ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 46 insertions(+)
>>>>>>>
>>>>>>> Hi David,
>>>>>>>
>>>>>>> At first glance this looks reasonable to me, I just have a two
>>>>>>> questions/comments:
>>>>>>>
>>>>>>> * Can you explain your current need for this functionality?  For
>>>>>>> example, what are you trying to do that is made easier by allowing
>>>>>>> greater message queue labeling flexibility?  This helps put things in
>>>>>>> context and helps people review and comment on your patch.
>>>>>>>
>>>>>>> * How have you tested this?  While this patch is not SELinux specific,
>>>>>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>>>>>> The other LSM maintainers may suggest something similar if they have
>>>>>>> an established public testsuite.
>>>>>>>
>>>>>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>>>>>
>>>>>> Hi Paul,
>>>>>>
>>>>>> I needed to write a selinux policy for a set of custom applications that use
>>>>>> POSIX message queues for their IPC. The queues are created by one
>>>>>> application and we needed a way for selinux to enforce which of the other
>>>>>> apps are able to read/write to each individual queue. Uniquely labeling them
>>>>>> based on the app that created them and the file name seemed to be our best
>>>>>> solution since it’s an embedded system and we don’t have restorecond to
>>>>>> handle any relabeling.
>>>>>
>>>>> In the future putting things like the above in the patch description
>>>>> can be helpful.  In other words, instead of simply saying this allows
>>>>> you to better control the labels assigned to message queues, you could
>>>>> expand upon it by saying that this patch allows you to better control
>>>>> which applications have access to a given queue.  Yes, I realize that
>>>>> is implied by better control over the labels, but being explicit is
>>>>> rarely a bad thing when it comes to patch descriptions.
>>>>>
>>>>> I've never rejected a patch for a description that was too lengthy,
>>>>> but I have rejected patches that need better descriptions ;)
>>>>>
>>>>>> To test this patch I used both a selinux enabled, buildroot based qemu target
>>>>>> with a customized selinux policy and test C app to create the mqueues. I also
>>>>>> tested with our real apps and selinux policy on our target hardware. I can
>>>>>> certainly look at adding a test to the selinux-testsuite if that would
>>>>>> be helpful.
>>>>>
>>>>> Please do.  I've been requiring tests for all new SELinux
>>>>> functionality lately; this isn't strictly a SELinux patch but I think
>>>>> it is a good practice regardless.
>>>>
>>>> Sorry for the delay. I have created a pull request within the
>>>> selinux-testsuite github
>>>> project with a set of mqueue tests.
>>>
>>> For anyone who is curious:
>>>
>>> * https://github.com/SELinuxProject/selinux-testsuite/pull/10
>>>
>>> Aside from a naming nit, the tests look good to me and I have no
>>> problem with the kernel patch; it doesn't appear any of the other LSM
>>> maintainers do either.  I'm happy to pull this into the SELinux tree
>>> (for v4.11, it's a little late for v4.10 I think), but I think
>>> Christoph made a good point about consolidation, have you had a chance
>>> to look at that?
>>
>> I've made the update for the naming nit in the pull request.
>
> I saw that, thanks.
>
>> I agree with Christoph's point but doing so is a bit outside my expertise at
>> this point. I would be open to suggestions as to where the function should be
>> consolidated and work on a second patchset with the update. Maybe in
>> fs/xattr.c as a simple_xattr_initxattrs function?
>
> That seems to make the most sense, doesn't it?  Looking at
> {shmem,mqueue}_initxattrs() the only fs specific bit is the
> {shmem,mqueue}_inode_info struct pointer; considering that the fs_info
> parameter is currently unused in this case, you could pass a reference
> to the simple_xattr struct via the fs_info parameter.
>
> I'd CC Andreas Gruenbacher <agruenba@redhat.com> on the patch(set),
> he's recently done a bunch of work around xattrs and the LSM, he may
> have some additional thoughts.
>
Thanks for the advice. I'm testing a patchset with the proposed changes and
planning to submit them later today.