From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753425AbcLEXNE (ORCPT ); Mon, 5 Dec 2016 18:13:04 -0500 Received: from mail-vk0-f68.google.com ([209.85.213.68]:35600 "EHLO mail-vk0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753229AbcLEXNC (ORCPT ); Mon, 5 Dec 2016 18:13:02 -0500 MIME-Version: 1.0 X-Originating-IP: [96.230.190.88] In-Reply-To: References: <1478551587-33892-1-git-send-email-david.graziano@rockwellcollins.com> From: Paul Moore Date: Mon, 5 Dec 2016 18:12:55 -0500 Message-ID: Subject: Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support To: David Graziano Cc: golbi@mat.uni.torun.pl, 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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uB5NDABd021433 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. -- paul moore www.paul-moore.com