Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200206165527.211350-1-smoreland@google.com> <91465612-2fb2-5985-ba45-d4d9fcf0f70c@tycho.nsa.gov>
 <c61fc8f6-55c2-c717-5090-e535b7bdbb4f@tycho.nsa.gov> <CAKLm694DMH0JCpHuT4HgMd4yCNJZPFMpex8iEiRF9kRjPb0d6g@mail.gmail.com>
 <a8321785-902d-9186-fcf5-ee12a362a207@tycho.nsa.gov> <CAHC9VhQVcgQ7ZKXi+Umm51WNgfNSzNX7Zoe=MyaLo1rRm-uUtA@mail.gmail.com>
In-Reply-To: <CAHC9VhQVcgQ7ZKXi+Umm51WNgfNSzNX7Zoe=MyaLo1rRm-uUtA@mail.gmail.com>
From:   Steven Moreland <smoreland@google.com>
Date:   Wed, 12 Feb 2020 09:46:13 -0800
Message-ID: <CAKLm695ZVCfdTnyA4UjaWeQmP3gGNjk_kMj1YOdV+kAbcvSrBw@mail.gmail.com>
Subject: Re: [PATCH] security: selinux: allow per-file labeling for bpffs
To:     Paul Moore <paul@paul-moore.com>
Cc:     Stephen Smalley <sds@tycho.nsa.gov>,
        Colin Cross <ccross@android.com>,
        "Connor O'Brien" <connoro@google.com>, kernel-team@android.com,
        Eric Paris <eparis@parisplace.org>,
        Kees Cook <keescook@chromium.org>, anton@enomsg.org,
        tony.luck@intel.com, selinux@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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