Message-ID: <559FC421.3000109@tycho.nsa.gov>
Date:	Fri, 10 Jul 2015 09:09:53 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	Hugh Dickins <hughd@...gle.com>
CC:	Stephen Smalley <stephen.smalley@...il.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Morten Stevens <mstevens@...oraproject.org>,
	Eric Sandeen <esandeen@...hat.com>,
	Dave Chinner <david@...morbit.com>,
	Daniel Wagner <wagi@...om.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Eric Paris <eparis@...hat.com>, linux-mm@...ck.org,
	selinux <selinux@...ho.nsa.gov>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	David Howells <dhowells@...hat.com>
Subject: Re: mm: shmem_zero_setup skip security check and lockdep conflict
 with XFS

On 07/10/2015 03:48 AM, Hugh Dickins wrote:
> 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 <hughd@...gle.com> 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.

S_PRIVATE is supposed to disable all security processing on the inode,
and often this is checked in the security framework
(security/security.c) even before we reach the SELinux hook and causes
an immediate return there.  In the case of mprotect, we do reach the
SELinux code since the hook is on the vma, not merely the inode, so we
could apply an execmem check in the SELinux code if IS_PRIVATE() instead
of file execute.

However, I was trying to figure out if the fact that S_PRIVATE also
would disable any read/write checking by SELinux on the inode could
potentially open up a bypass of security policy.  That would only be an
issue if the file returned by shmem_zero_setup() was ever linked to an
open file descriptor that could be inherited across a fork+exec or
passed across local socket IPC or binder IPC and thereby shared across
different security contexts. Uses of shmem_zero_setup() include mmap
MAP_ANONYMOUS|MAP_SHARED, drivers/staging/android/ashmem.c (from
ashmem_mmap if VM_SHARED), and drivers/char/mem.c (from mmap_zero if
VM_SHARED).  That all seems fine AFAICS.

> (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.

Yes, I was hoping for that.  Ok, I'll spin up a patch for adding an
IS_PRIVATE() test to SELinux file_map_prot_check() and cc you all on it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists