Message-ID: <CAHC9VhSQTz3-0ONEccA_YfJ4fxWXOqRCQeocf7uPJOJ_jiXsFg@mail.gmail.com>
Date:   Thu, 12 Jan 2023 16:54:35 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     "T.J. Mercier" <tjmercier@...gle.com>
Cc:     Jeffrey Vander Stoep <jeffv@...gle.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Carlos Llamas <cmllamas@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, hannes@...xchg.org,
        daniel.vetter@...ll.ch, android-mm@...gle.com, jstultz@...gle.com,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

On Thu, Jan 12, 2023 at 4:36 PM T.J. Mercier <tjmercier@...gle.com> wrote:
> On Thu, Jan 12, 2023 at 12:45 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Wed, Jan 11, 2023 at 7:21 PM T.J. Mercier <tjmercier@...gle.com> wrote:
> > > On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@...l-moore.com> wrote:
> > > > On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@...gle.com> wrote:
> > > > >
> > > > > Any process can cause a memory charge transfer to occur to any other
> > > > > process when transmitting a file descriptor through binder. This should
> > > > > only be possible for central allocator processes, so a new SELinux
> > > > > permission is added to restrict which processes are allowed to initiate
> > > > > these charge transfers.
> > > > >
> > > > > Signed-off-by: T.J. Mercier <tjmercier@...gle.com>
> > > > > ---
> > > > >  drivers/android/binder.c            | 5 +++++
> > > > >  include/linux/lsm_hook_defs.h       | 2 ++
> > > > >  include/linux/lsm_hooks.h           | 6 ++++++
> > > > >  include/linux/security.h            | 2 ++
> > > > >  security/security.c                 | 6 ++++++
> > > > >  security/selinux/hooks.c            | 9 +++++++++
> > > > >  security/selinux/include/classmap.h | 2 +-
> > > > >  7 files changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > Hi T.J.,
> > > >
> > > > A few things come to mind when looking at this patchset, but let me
> > > > start with the big one first: you only sent 0/4 and 4/4 to the LSM and
> > > > SELinux lists, so that's all I'm seeing in my inbox to review, and
> > > > it's hard to make sense of what you want to do with just these
> > > > snippets.  This makes me cranky, and less inclined to spend the time
> > > > to give this a proper review, because there are plenty of other things
> > > > which need attention and don't require me having to hunt down missing
> > > > pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
> > > > workflow was pretty well established before they came into existence
> > > > and I still do things the good ol' fashioned way with mailing lists,
> > > > etc.
> > > >
> > > > Make the patch reviewer's life easy whenever you can, it will rarely
> > > > (ever?) backfire, I promise.
> > >
> > > Hi Paul, sorry about that. I have git send-email calling
> > > get_maintainer.pl to automatically figure out the recipients, and I
> > > think that's why it only sent particular patches to a subset of lists.
> > > Looks like the list of recipients for each patch should be a union of
> > > all patches. Thank you for taking a look anyway! Here's a lore link:
> > > https://lore.kernel.org/lkml/20230109213809.418135-1-tjmercier@google.com/
> > >
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index 9830848c8d25..9063db04826d 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> > > > >         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > > > >                 struct dma_buf *dmabuf;
> > > > >
> > > > > +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > > > > +                       ret = -EPERM;
> > > > > +                       goto err_security;
> > > > > +               }
> > > >
> > > > This is where I believe I'm missing the proper context, as this
> > > > version of binder_translate_fd() differs from what I see in Linus'
> > > > tree.  However, the version in Linus' tree does have a LSM hook,
> > > > security_binder_transfer_file(), which is passed both the credentials
> > > > you are using above and based solely on the level of indentation shown
> > > > in the chunk of code above, it seems like the existing hook might be
> > > > suitable?
> > >
> > > Yes, patch 3 plumbs through flags to this function:
> > > https://lore.kernel.org/lkml/20230109213809.418135-4-tjmercier@google.com/
> > >
> > > I don't think the existing hook is suitable, which I've tried to explain below.
> >
> > In this particular case the issue of what permission checks are done
> > for a given LSM, SELinux in this case, appears to be independent of if
> > we need a new, different, or second LSM hook.  Unless I missed
> > something the only real difference with this new hook is that is sits
> > behind a conditional checking if memory control groups are enabled and
> > if a transfer charge was specified; it seems like passing the @flags
> > parameter into the existing LSM hook would allow you to use the
> > existing hook (it is called before the new hook, right?)?
> >
> Ah yes, that sounds like it would work. Thank you.
>
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 3c5be76a9199..823ef14924bd 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> > > > >                             &ad);
> > > > >  }
> > > > >
> > > > > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > > > > +{
> > > > > +       return avc_has_perm(&selinux_state,
> > > > > +                           cred_sid(from), cred_sid(to),
> > > > > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > > > > +                           NULL);
> > > > > +}
> > > >
> > > > Generally speaking SELinux doesn't really worry about resource
> > > > accounting controls so this seems a bit out of place, but perhaps the
> > > > larger question is do you see this being sufficiently distinct from
> > > > the existing binder:transfer permission?  In other words, would you
> > > > ever want to grant a domain the ability to transfer a file *without*
> > > > also granting it the ability to transfer the memory charge?  You need
> > > > to help me explain why we need an additional permission for this,
> > > > because I don't currently see the need.
> > > >
> > > Yes, and that's actually more often the case than not. A file here
> > > means a file descriptor that points at any type of resource: file on
> > > disk, memfd, dmabuf, etc. Currently there exists policy that restricts
> > > which processes are allowed to interact with FDs over binder using the
> > > security_binder_transfer_file hook you reference. [1] However this new
> > > transfer_charge permission is meant to restrict the ability of a FD
> > > sender to transfer the memory charge associated with that FD (if one
> > > exists) to a recipient (who may or may not want to accept the memory
> > > charge). So the memory charge is independent of (potentially one-time,
> > > read-only) access to the FD.
> >
> > Without a more comprehensive set of LSM/SELinux access controls around
> > resource management (which would need discussion beyond just this
> > thread/patch) I'm not sure we want to start patching in one-off
> > controls like this.
>
> Understood, I'll try reusing security_binder_transfer_file.

Well, yes, it looks like you should reuse the existing hook, but that
is missing the point here - I'm not convinced we want this new SELinux
control without some additional discussion and work around the
desirability and practicality of adding SELinux controls to resource
management.

> > I haven't looked, but are there any traditional/DAC access controls
> > around transfering memory changes from one task to another?  It seems
> > like there *should* be, and if so, it seems like that would be the
> > right approach at the moment ... if you're not already doing that in
> > the other patches in the patchset.
>
> I'm not aware of controls associated with individual objects like
> dmabufs. While it's not quite the same thing, I do see that support
> for charge migration tied to task migration was intentionally dropped
> for cgroup2 and is now deprecated for cgroup1 because it's difficult
> and expensive. However that seems like a much bigger job than dealing
> with the memory backing an individual object when that object is
> handed off between processes (the object ownership moves, not the
> task).

I would suggest you should start talking to the cgroup folks about
this, that seems like a much better first step than jumping straight
to a LSM hook to control just one type of charge transfer.  At the
very least, that is the start of the much larger conversation that
would be needed to add resource management controls to SELinux (and
other LSMs that wanted to implement similar controls).

-- 
paul-moore.com

Powered by blists - more mailing lists