Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753460AbbGJHse (ORCPT ); Fri, 10 Jul 2015 03:48:34 -0400 Received: from mail-pd0-f172.google.com ([209.85.192.172]:33669 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753204AbbGJHs0 (ORCPT ); Fri, 10 Jul 2015 03:48:26 -0400 Date: Fri, 10 Jul 2015 00:48:14 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Stephen Smalley cc: Hugh Dickins , Stephen Smalley , Prarit Bhargava , Morten Stevens , Eric Sandeen , Dave Chinner , Daniel Wagner , Linux Kernel , Eric Paris , linux-mm@kvack.org, selinux , Andrew Morton , Linus Torvalds , David Howells Subject: Re: mm: shmem_zero_setup skip security check and lockdep conflict with XFS In-Reply-To: <559E7023.8040203@tycho.nsa.gov> Message-ID: References: <559D51C2.7060603@tycho.nsa.gov> <559E7023.8040203@tycho.nsa.gov> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4608 Lines: 84 On Thu, 9 Jul 2015, Stephen Smalley wrote: > On 07/09/2015 04:23 AM, Hugh Dickins wrote: > > On Wed, 8 Jul 2015, Stephen Smalley wrote: > >> On 07/08/2015 09:13 AM, Stephen Smalley wrote: > >>> On Sun, Jun 14, 2015 at 12:48 PM, Hugh Dickins wrote: > >>>> It appears that, at some point last year, XFS made directory handling > >>>> changes which bring it into lockdep conflict with shmem_zero_setup(): > >>>> it is surprising that mmap() can clone an inode while holding mmap_sem, > >>>> but that has been so for many years. > >>>> > >>>> Since those few lockdep traces that I've seen all implicated selinux, > >>>> I'm hoping that we can use the __shmem_file_setup(,,,S_PRIVATE) which > >>>> v3.13's commit c7277090927a ("security: shmem: implement kernel private > >>>> shmem inodes") introduced to avoid LSM checks on kernel-internal inodes: > >>>> the mmap("/dev/zero") cloned inode is indeed a kernel-internal detail. > >>>> > >>>> This also covers the !CONFIG_SHMEM use of ramfs to support /dev/zero > >>>> (and MAP_SHARED|MAP_ANONYMOUS). I thought there were also drivers > >>>> which cloned inode in mmap(), but if so, I cannot locate them now. > >>> > >>> This causes a regression for SELinux (please, in the future, cc > >>> selinux list and Paul Moore on SELinux-related changes). In > > > > Surprised and sorry about that, yes, I should have Cc'ed. > > > >>> particular, this change disables SELinux checking of mprotect > >>> PROT_EXEC on shared anonymous mappings, so we lose the ability to > >>> control executable mappings. That said, we are only getting that > >>> check today as a side effect of our file execute check on the tmpfs > >>> inode, whereas it would be better (and more consistent with the > >>> mmap-time checks) to apply an execmem check in that case, in which > >>> case we wouldn't care about the inode-based check. However, I am > >>> unclear on how to correctly detect that situation from > >>> selinux_file_mprotect() -> file_map_prot_check(), because we do have a > >>> non-NULL vma->vm_file so we treat it as a file execute check. In > >>> contrast, if directly creating an anonymous shared mapping with > >>> PROT_EXEC via mmap(...PROT_EXEC...), selinux_mmap_file is called with > >>> a NULL file and therefore we end up applying an execmem check. > > > > If you're willing to go forward with the change, rather than just call > > for an immediate revert of it, then I think the right way to detect > > the situation would be to check IS_PRIVATE(file_inode(vma->vm_file)), > > wouldn't it? > > That seems misleading and might trigger execmem checks on non-shmem > inodes. S_PRIVATE was originally introduced for fs-internal inodes that > are never directly exposed to userspace, originally for reiserfs xattr > inodes (reiserfs xattrs are internally implemented as their own files > that are hidden from userspace) and later also applied to anon inodes. > It would be better if we had an explicit way of testing that we are > dealing with an anonymous shared mapping in selinux_file_mprotect() -> > file_map_prot_check(). But how would any of those original S_PRIVATE inodes arrive at selinux_file_mprotect()? Now we have added the anon shared mmap case which can arrive there, but the S_PRIVATE check seems just the right tool for the job of distinguishing those from the user-visible inodes. I don't see how adding some other flag for this case would be better - though certainly I can see that adding an "anon shared shmem" comment on its use in that check would be helpful. Or is there some further difficulty in this use of S_PRIVATE, beyond the mprotect case that you've mentioned? Unless there is some further difficulty, duplicating all the code relating to S_PRIVATE for a differently named flag seems counter-productive to me. (There is a bool shmem_mapping(mapping) that could be used to confirm that the inode you're looking at indeed belongs to shmem; but of course that would say yes on all the user-visible shmem inodes too, so it wouldn't be a useful test on its own, and I don't see that adding it to an S_PRIVATE test would add any real value.) Probably you were hoping that there's already some distinguishing feature of anon shared shmem inodes that you could check: I can't think of one offhand, beyond S_PRIVATE: if there is another, it would be accidental. Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/