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@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>> <david.graziano@xxxxxxxxxxxxxxxxxxx> wrote:
>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>> <david.graziano@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>>  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