Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753663AbcLHPQz convert rfc822-to-8bit (ORCPT ); Thu, 8 Dec 2016 10:16:55 -0500 Received: from secvs02.rockwellcollins.com ([205.175.225.241]:24021 "EHLO secvs02.rockwellcollins.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751590AbcLHPQy (ORCPT ); Thu, 8 Dec 2016 10:16:54 -0500 X-RC-All-From: , 205.175.225.133, No hostname, david.graziano@rockwellcollins.com, David Graziano , , X-RC-Attachments: , , X-RC-RemoteIP: 205.175.225.133 X-RC-RemoteHost: No hostname X-RC-IP-Hostname: secip02.rockwellcollins.com X-RC-IP-MID: 163442680 X-RC-IP-Group: GOOGLE_RELAYED X-RC-IP-Policy: $GOOGLE_RELAYED X-RC-IP-SBRS: None MIME-Version: 1.0 In-Reply-To: References: <1478551587-33892-1-git-send-email-david.graziano@rockwellcollins.com> From: David Graziano Date: Thu, 8 Dec 2016 09:16:30 -0600 Message-ID: Subject: Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support To: Paul Moore Cc: Michal Wronski , linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, Stephen Smalley , Seth Forshee , ebiederm@xmission.com, selinux@tycho.nsa.gov Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5523 Lines: 108 On Mon, Dec 5, 2016 at 5:12 PM, Paul Moore wrote: > On Mon, Dec 5, 2016 at 11:15 AM, David Graziano > wrote: >> On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore wrote: >>> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano >>> wrote: >>>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore wrote: >>>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano >>>>> wrote: >>>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore wrote: >>>>>>> On Mon, Nov 7, 2016 at 3:46 PM, 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 >>>>>>>> --- >>>>>>>> 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 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.