Message-ID: <CABdmKX08sg2+MtzHD2ar7eD8xXNGUbbS03zYSMpK+wF51LztmQ@mail.gmail.com>
Date:   Thu, 12 Jan 2023 13:36:27 -0800
From:   "T.J. Mercier" <tjmercier@...gle.com>
To:     Paul Moore <paul@...l-moore.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 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.

> 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).

https://lore.kernel.org/all/20221206171340.139790-4-hannes@cmpxchg.org/


> --
> paul-moore.com

Powered by blists - more mailing lists