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

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

[Steven Moreland]

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.

Change-Id: I03ae28d3afea70acd6dc53ebf810b34b357b6eb5
Signed-off-by: Connor O'Brien <connoro@google.com>
Signed-off-by: Steven Moreland <smoreland@google.com>
---
 security/selinux/hooks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index de4887742d7c..4f9396e6ce8c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -872,6 +872,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 	    !strcmp(sb->s_type->name, "sysfs") ||
 	    !strcmp(sb->s_type->name, "pstore") ||
 	    !strcmp(sb->s_type->name, "binder") ||
+	    !strcmp(sb->s_type->name, "bpf") ||
 	    !strcmp(sb->s_type->name, "cgroup") ||
 	    !strcmp(sb->s_type->name, "cgroup2"))
 		sbsec->flags |= SE_SBGENFS;
-- 
2.25.0.341.g760bfbb309-goog

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

[Stephen Smalley]

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.

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

> Change-Id: I03ae28d3afea70acd6dc53ebf810b34b357b6eb5

Drop Change-Ids from patches submitted upstream please since they aren't 
meaningful outside of Android.

> Signed-off-by: Connor O'Brien <connoro@google.com>
> Signed-off-by: Steven Moreland <smoreland@google.com>
> ---
>   security/selinux/hooks.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index de4887742d7c..4f9396e6ce8c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -872,6 +872,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>   	    !strcmp(sb->s_type->name, "sysfs") ||
>   	    !strcmp(sb->s_type->name, "pstore") ||
>   	    !strcmp(sb->s_type->name, "binder") ||
> +	    !strcmp(sb->s_type->name, "bpf") ||
>   	    !strcmp(sb->s_type->name, "cgroup") ||
>   	    !strcmp(sb->s_type->name, "cgroup2"))
>   		sbsec->flags |= SE_SBGENFS;
> 

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

[Stephen Smalley]

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.
> 
> 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
> 
>> Change-Id: I03ae28d3afea70acd6dc53ebf810b34b357b6eb5
> 
> Drop Change-Ids from patches submitted upstream please since they aren't 
> meaningful outside of Android.
> 
>> Signed-off-by: Connor O'Brien <connoro@google.com>
>> Signed-off-by: Steven Moreland <smoreland@google.com>
>> ---
>>   security/selinux/hooks.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index de4887742d7c..4f9396e6ce8c 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -872,6 +872,7 @@ static int selinux_set_mnt_opts(struct super_block 
>> *sb,
>>           !strcmp(sb->s_type->name, "sysfs") ||
>>           !strcmp(sb->s_type->name, "pstore") ||
>>           !strcmp(sb->s_type->name, "binder") ||
>> +        !strcmp(sb->s_type->name, "bpf") ||
>>           !strcmp(sb->s_type->name, "cgroup") ||
>>           !strcmp(sb->s_type->name, "cgroup2"))
>>           sbsec->flags |= SE_SBGENFS;
>>

Also, your patch appears to be based on an old kernel and won't apply 
upstream; see
https://github.com/SELinuxProject/selinux-kernel/blob/master/README.md

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

[Steven Moreland]

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?

> >> Change-Id: I03ae28d3afea70acd6dc53ebf810b34b357b6eb5
> >
> > Drop Change-Ids from patches submitted upstream please since they aren't
> > meaningful outside of Android.
> >

Yeah, will resubmit, thanks.

> >> Signed-off-by: Connor O'Brien <connoro@google.com>
> >> Signed-off-by: Steven Moreland <smoreland@google.com>
> >> ---
> >>   security/selinux/hooks.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index de4887742d7c..4f9396e6ce8c 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -872,6 +872,7 @@ static int selinux_set_mnt_opts(struct super_block
> >> *sb,
> >>           !strcmp(sb->s_type->name, "sysfs") ||
> >>           !strcmp(sb->s_type->name, "pstore") ||
> >>           !strcmp(sb->s_type->name, "binder") ||
> >> +        !strcmp(sb->s_type->name, "bpf") ||
> >>           !strcmp(sb->s_type->name, "cgroup") ||
> >>           !strcmp(sb->s_type->name, "cgroup2"))
> >>           sbsec->flags |= SE_SBGENFS;
> >>
>
> Also, your patch appears to be based on an old kernel and won't apply
> upstream; see
> https://github.com/SELinuxProject/selinux-kernel/blob/master/README.md
>

Will resubmit, thanks.

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

[Stephen Smalley]

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.

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

[Paul Moore]

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

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

[Steven Moreland]

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?


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

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

[Stephen Smalley]

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