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@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 6, 2020 at 1:12 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> > On 2/6/20 12:41 PM, Steven Moreland wrote:
> > > On Thu, Feb 6, 2020 at 9:35 AM Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > >>>>
> > >>>> 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