Message-ID: <20181221141241.gnxoiw7t5ajwcd6d@brauner.io>
Date:   Fri, 21 Dec 2018 15:12:42 +0100
From:   Christian Brauner <christian@...uner.io>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     tkjos@...roid.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, joel@...lfernandes.org,
        arve@...roid.com, maco@...roid.com
Subject: Re: [PATCH] binderfs: implement sysctls

On Fri, Dec 21, 2018 at 02:55:09PM +0100, Greg KH wrote:
> On Fri, Dec 21, 2018 at 02:39:09PM +0100, Christian Brauner wrote:
> > This implements three sysctls that have very specific goals:
> 
> Ick, why?
> 
> What are these going to be used for?  Who will "control" them?  As you

Only global root in the initial user namespace. See the reasons below. :)

> are putting them in the "global" namespace, that feels like something
> that binderfs was trying to avoid in the first place.

There are a couple of reason imho:
- Global root needs a way to restrict how many binder devices can be
  allocated across all user + ipc namespace pairs.
  One obvious reason is that otherwise userns root in a non-initial user
  namespace can allocate a huge number of binder devices (pick a random
  number say 10.000) and use up a lot of kernel memory.
  In addition they can pound on the binder.c code causing a lot of
  contention for the remaining global lock in there.
  We should let global root explicitly restrict non-initial namespaces
  in this respect. Imho, that's just good security design. :)
- The reason for having a number of reserved devices is when the initial
  binderfs mount needs to bump the number of binder devices after the
  initial allocation done during say boot (e.g. it could've removed
  devices and wants to reallocate new ones but all binder minor numbers
  have been given out or just needs additional devices). By reserving an
  initial pool of binder devices this can be easily accounted for and
  future proofs userspace. This is to say: global root in the initial
  userns + ipcns gets dibs on however many devices it wants. :)
- The fact that we have a single shared pool of binder device minor
  numbers for all namespaces imho makes it necessary for the global root
  user in the initial ipc + user namespace to manage device allocation
  and delegation.

The binderfs sysctl stuff is really small code-wise and adds a lot of
security without any performance impact on the code itself. So we
actually very strictly adhere to the requirement to not blindly
sacrifice performance for security. :)

> 
> > 1. /proc/sys/fs/binder/max:
> >    Allow global root to globally limit the number of allocatable binder
> >    devices.
> 
> Why?  Who cares?  Memory should be your only limit here, and when you
> run into that limit, you will start failing :)
> 
> > 2. /proc/sys/fs/binder/nr:
> >    Allow global root to easily detect how many binder devices are currently
> >    in use across all binderfs mounts.
> 
> Why?  Again, who cares?
> 
> > 3. /proc/sys/fs/binder/reserved:
> >    Ensure that global root can reserve binder devices for the initial
> >    binderfs mount in the initial ipc namespace to prevent DOS attacks.
> 
> Huh?  Can't you just create your "global root" devices first?  Doesn't
> the code do that already anyway?
> 
> And what kind of DoS attack could this ever prevent from anyway?
> 
> > This is equivalent to sysctls of devpts.
> 
> devpts isn't exactly the best thing to emulate :)

It's one of its saner design choices though. :)

> 
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> > ---
> >  drivers/android/binderfs.c | 81 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 79 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 7496b10532aa..5ff015f82314 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -64,6 +64,71 @@ struct binderfs_info {
> >  
> >  };
> >  
> > +/* Global default limit on the number of binder devices. */
> > +static int device_limit = 4096;
> > +
> > +/*
> > + * Number of binder devices reserved for the initial binderfs mount in the
> > + * initial ipc namespace.
> > + */
> > +static int device_reserve = 1024;
> > +
> > +/* Dummy sysctl minimum. */
> > +static int device_limit_min;
> > +
> > +/* Cap sysctl at BINDERFS_MAX_MINOR. */
> > +static int device_limit_max = BINDERFS_MAX_MINOR;
> > +
> > +/* Current number of allocated binder devices. */
> > +static atomic_t device_count = ATOMIC_INIT(0);
> 
> You have a lock you are using, just rely on that, don't create
> yet-another-type-of-unneeded-lock with an atomic here.
> 
> Anyway, I really don't see the need for any of this just yet, so I
> didn't read beyond this point in the code :)
> 
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists