Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531596 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support David Graziano <david.graziano@rockwellcollins.com> - 2016-11-28 21:10 +0100
    Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support Paul Moore <paul@paul-moore.com> - 2016-12-05 16:40 +0100
      Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support David Graziano <david.graziano@rockwellcollins.com> - 2016-12-05 17:20 +0100
        Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support Paul Moore <paul@paul-moore.com> - 2016-12-06 00:20 +0100
          Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support David Graziano <david.graziano@rockwellcollins.com> - 2016-12-08 16:20 +0100

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

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

[toc] | [next] | [standalone]

#1536177

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

[toc] | [prev] | [next] | [standalone]

#1536204

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

[toc] | [prev] | [next] | [standalone]

#1536520

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

[toc] | [prev] | [next] | [standalone]

#1538603

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web