Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.

[Christian Brauner]

On Wed, Sep 04, 2019 at 09:19:29AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2019 at 11:41:12AM -0700, Hridya Valsaraju wrote:
> > On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
> > <christian.brauner@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > > > Length of a binderfs device name cannot exceed BINDERFS_MAX_NAME.
> > > > > This patch adds a check in binderfs_init() to ensure the same
> > > > > for the default binder devices that will be created in every
> > > > > binderfs instance.
> > > > >
> > > > > Co-developed-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > > > > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > > > > Signed-off-by: Hridya Valsaraju <hridya@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/android/binderfs.c | 12 ++++++++++++
> > > > >  1 file changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > > > index aee46dd1be91..55c5adb87585 100644
> > > > > --- a/drivers/android/binderfs.c
> > > > > +++ b/drivers/android/binderfs.c
> > > > > @@ -570,6 +570,18 @@ static struct file_system_type binder_fs_type = {
> > > > >  int __init init_binderfs(void)
> > > > >  {
> > > > >     int ret;
> > > > > +   const char *name;
> > > > > +   size_t len;
> > > > > +
> > > > > +   /* Verify that the default binderfs device names are valid. */
> > > >
> > > > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> > > >
> > > > > +   name = binder_devices_param;
> > > > > +   for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > > > > +           if (len > BINDERFS_MAX_NAME)
> > > > > +                   return -E2BIG;
> > > > > +           name += len;
> > > > > +           if (*name == ',')
> > > > > +                   name++;
> > > > > +   }
> > > >
> > > > We already tokenize the binderfs device names in binder_init(), why not
> > > > check this there instead?  Parsing the same string over and over isn't
> > > > the nicest.
> > >
> > > non-binderfs binder devices do not have their limit set to
> > > BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> > > binderfs binder devices which do have that limit.
> > 
> > 
> > Thank you Greg and Christian, for taking another look. Yes,
> > non-binderfs binder devices not having this limitation is the reason
> > why the check was made specific to binderfs devices. Also, when
> > CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
> > being parsed in binder_init().
> > 
> > >
> > > But, in practice, 255 is the standard path-part limit that no-one really
> > > exceeds especially not for stuff such as device nodes which usually have
> > > rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> > > So yes, we can move that check before both the binderfs binder device
> > > and non-binderfs binder device parsing code and treat it as a generic
> > > check.
> > > Then we can also backport that check as you requested in the other mail.
> > > Unless Hridya or Todd have objections, of course.
> > 
> > I do not have any objections to adding a generic check in binder_init() instead.
> 
> Was this patchset going to be redone based on this?

No, we decided to leave this check specific to binderfs for now because
the length limit only applies to binderfs devices. If you really want to
have this check in binder we can send a follow-up. I would prefer to
take the series as is.

Btw, for the two binderfs series from Hridya, do you want me to get a
branch ready and send you a PR for both of them together?

Christian