Re: [PATCH] security: selinux: allow per-file labeling for bpffs

[Stephen Smalley <sds@tycho.nsa.gov>]

On 2/12/20 12:46 PM, Steven Moreland wrote:
> And I strongly encourage our downstream in the same way :) I try, I
> try. However, I am a n00b here (thanks for merging "my" first linux
> patch).
> 
> Looking at this code, I was wondering, why isn't SELinux labelling
> completely orthogonal to the fs type? Is this a measurable
> memory/performance thing?

If you just mean why don't we turn on SE_SBGENFS for all filesystem 
types, that's discussed in
https://github.com/SELinuxProject/selinux-kernel/issues/2

It isn't always safe so we have been whitelisting the filesystem types 
that are supported.

More generally, labeling in SELinux goes beyond just GENFS; there are 
the SECURITY_FS_USE_* filesystem labeling behaviors defined by policy 
and those are also based on fstype, just not hardcoded in the kernel.

> 
> 
> On Tue, Feb 11, 2020 at 7:17 PM Paul Moore <paul@paul-moore.com> wrote:
>>
>> On Thu, Feb 6, 2020 at 1:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 2/6/20 12:41 PM, Steven Moreland wrote:
>>>> On Thu, Feb 6, 2020 at 9:35 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>
>>>>> On 2/6/20 12:21 PM, Stephen Smalley wrote:
>>>>>> On 2/6/20 11:55 AM, Steven Moreland wrote:
>>>>>>> From: Connor O'Brien <connoro@google.com>
>>>>>>>
>>>>>>> Add support for genfscon per-file labeling of bpffs files. This allows
>>>>>>> for separate permissions for different pinned bpf objects, which may
>>>>>>> be completely unrelated to each other.
>>>>>>
>>>>>> Do you want bpf fs to also support userspace labeling of files via
>>>>>> setxattr()?  If so, you'll want to also add it to
>>>>>> selinux_is_genfs_special_handling() as well.
>>>>>>
>>>>
>>>> Android doesn't currently have this use case.
>>>>
>>>>>> The only caveat I would note here is that it appears that bpf fs
>>>>>> supports rename, link, unlink, rmdir etc by userspace, which means that
>>>>>> name-based labeling via genfscon isn't necessarily safe/stable.  See
>>>>>> https://github.com/SELinuxProject/selinux-kernel/issues/2
>>>>>>
>>>>
>>>> Android restricts ownership of these files to a single process (bpfloader) and
>>>> so this isn't a concern in our architecture. Is it a concern in general?
>>>
>>> I guess if the inodes are pinned in memory, then only the original name
>>> under which the file is created will be relevant to determining the
>>> label and subsequent rename/link operations won't have any effect. So as
>>> long as the bpfloader creates the files with the same names being
>>> specified in policy, that should line up and be stable for the lifecycle
>>> of the inode.
>>>
>>> The alternative model is to have bpfloader look up a context from the
>>> userspace file_contexts configuration via selabel_lookup(3) and friends,
>>> and set it on the file explicitly.  That's what e.g. ueventd does for
>>> device nodes.  However, one difference here is that you could currently
>>> only do this via setxattr()/setfilecon() after creating the file so that
>>> the file would temporarily exist in the default label for bpf fs, if
>>> that matters.  ueventd can instead use setfscreatecon(3) before creating
>>> the file so that it is originally created in the right label but that
>>> requires the filesystem to call security_inode_init_security() from its
>>> function that originally creates the inode, which tmpfs/devtmpfs does
>>> but bpf does not.  So you'd have to add that to the bpf filesystem code
>>> if you wanted to support setfscreatecon(3) on it.
>>
>> Considering the relative maturity of bpf, and bpffs, I think it's okay
>> to take this small step right now, with the understanding that more
>> work may need to be done, depending on how this is generally adopted
>> by distros and users (for those of you not following the other thread,
>> I've merged the v3 draft of this patch).
>>
>> However, I've been noticing a trend from the Android folks of tossing
>> patches over the wall without much thought beyond the Android use
>> case.  I understand the Android devs have a job to do, and products to
>> focus on, but I would strongly encourage them to think a bit longer
>> about more general use cases before submitting patches upstream.
>>
>> --
>> paul moore
>> www.paul-moore.com