[PATCH v3 0/2] Add default binderfs devices

[PATCH v3 0/2] Add default binderfs devices

[Hridya Valsaraju]

Binderfs was created to help provide private binder devices to
containers in their own IPC namespace. Currently, every time a new binderfs
instance is mounted, its private binder devices need to be created via
IOCTL calls. This patch series eliminates the effort for creating
the default binder devices for each binderfs instance by creating them
automatically.

Hridya Valsaraju (2):
  binder: Add default binder devices through binderfs when configured
  binder: Validate the default binderfs device names.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 35 ++++++++++++++++++++++++++++---
 3 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Hridya Valsaraju]

Currently, since each binderfs instance needs its own
private binder devices, every time a binderfs instance is
mounted, all the default binder devices need to be created
via the BINDER_CTL_ADD IOCTL. This patch aims to
add a solution to automatically create the default binder
devices for each binderfs instance that gets mounted.
To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
are created in each binderfs instance instead of global devices
being created by the binder driver.

Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
---

Changes in v2:
- Updated commit message as per Greg Kroah-Hartman.
- Removed new module parameter creation as per Greg
  Kroah-Hartman/Christian Brauner.
- Refactored device name length check into a new patch as per Greg Kroah-Hartman.

Changes in v3:
-Removed unnecessary empty lines as per Dan Carpenter.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 466b6a7f8ab7..ca6b21a53321 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
 	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
 module_param_named(debug_mask, binder_debug_mask, uint, 0644);
 
-static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
 module_param_named(devices, binder_devices_param, charp, 0444);
 
 static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
@@ -6279,7 +6279,8 @@ static int __init binder_init(void)
 				    &transaction_log_fops);
 	}
 
-	if (strcmp(binder_devices_param, "") != 0) {
+	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+	    strcmp(binder_devices_param, "") != 0) {
 		/*
 		* Copy the module_parameter string, because we don't want to
 		* tokenize it in-place.
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 045b3e42d98b..fe8c745dc8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -37,6 +37,8 @@ struct binder_device {
 
 extern const struct file_operations binder_fops;
 
+extern char *binder_devices_param;
+
 #ifdef CONFIG_ANDROID_BINDERFS
 extern bool is_binderfs_device(const struct inode *inode);
 #else
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..aee46dd1be91 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
 	req->major = MAJOR(binderfs_dev);
 	req->minor = minor;
 
-	ret = copy_to_user(userp, req, sizeof(*req));
-	if (ret) {
+	if (userp && copy_to_user(userp, req, sizeof(*req))) {
 		ret = -EFAULT;
 		goto err;
 	}
@@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	int ret;
 	struct binderfs_info *info;
 	struct inode *inode = NULL;
+	struct binderfs_device device_info = { 0 };
+	const char *name;
+	size_t len;
 
 	sb->s_blocksize = PAGE_SIZE;
 	sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +523,22 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	if (!sb->s_root)
 		return -ENOMEM;
 
-	return binderfs_binder_ctl_create(sb);
+	ret = binderfs_binder_ctl_create(sb);
+	if (ret)
+		return ret;
+
+	name = binder_devices_param;
+	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+		strscpy(device_info.name, name, len + 1);
+		ret = binderfs_binder_device_create(inode, NULL, &device_info);
+		if (ret)
+			return ret;
+		name += len;
+		if (*name == ',')
+			name++;
+	}
+
+	return 0;
 }
 
 static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-- 
2.22.0.770.g0f2c4a37fd-goog

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

[Hridya Valsaraju]

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@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
---
 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. */
+	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++;
+	}
 
 	/* Allocate new major number for binderfs. */
 	ret = alloc_chrdev_region(&binderfs_dev, 0, BINDERFS_MAX_MINOR,
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Greg Kroah-Hartman]

On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote:
> Currently, since each binderfs instance needs its own
> private binder devices, every time a binderfs instance is
> mounted, all the default binder devices need to be created
> via the BINDER_CTL_ADD IOCTL.

Wasn't that a design goal of binderfs?

> This patch aims to
> add a solution to automatically create the default binder
> devices for each binderfs instance that gets mounted.
> To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
> the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
> are created in each binderfs instance instead of global devices
> being created by the binder driver.

This is going to change how things work today, what is going to break
because of this change?

I don't object to this, except for the worry of changing the default
behavior.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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@ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya@google.com>
> ---
>  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. */
> +	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++;
> +	}

Shouldn't this be a bugfix separate from patch 1/2?

And shouldn't it be backported to older kernels that currently have this
issue?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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@ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya@google.com>
> ---
>  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.

thanks,

greg k-h

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

[Christian Brauner]

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@ubuntu.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > ---
> >  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.

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.

Christian

Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Christian Brauner]

On Fri, Aug 09, 2019 at 04:50:16PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote:
> > Currently, since each binderfs instance needs its own
> > private binder devices, every time a binderfs instance is
> > mounted, all the default binder devices need to be created
> > via the BINDER_CTL_ADD IOCTL.
> 
> Wasn't that a design goal of binderfs?

Sure, but if you solely rely binderfs to be used to provide binder
devices having them pre-created on each mount makes quite some sense,
imho.

> 
> > This patch aims to
> > add a solution to automatically create the default binder
> > devices for each binderfs instance that gets mounted.
> > To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
> > the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
> > are created in each binderfs instance instead of global devices
> > being created by the binder driver.
> 
> This is going to change how things work today, what is going to break
> because of this change?
> 
> I don't object to this, except for the worry of changing the default
> behavior.

This is something that Hridya and Todd can speak better to given that
they suggested this change. :)
From my perspective, binderfs binder devices and the regular binder
driver are mostly used mutually exclusive in practice atm so that this
change has little chance of breaking anyone.

Now I really need to go back to vacation time - which I suck at
apparently. :)

Christian

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

[Hridya Valsaraju]

On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
<christian.brauner@ubuntu.com> 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@ubuntu.com>
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > ---
> > >  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.

>
> Christian

Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Hridya Valsaraju]

On Fri, Aug 9, 2019 at 11:22 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Aug 09, 2019 at 04:50:16PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote:
> > > Currently, since each binderfs instance needs its own
> > > private binder devices, every time a binderfs instance is
> > > mounted, all the default binder devices need to be created
> > > via the BINDER_CTL_ADD IOCTL.
> >
> > Wasn't that a design goal of binderfs?
>
> Sure, but if you solely rely binderfs to be used to provide binder
> devices having them pre-created on each mount makes quite some sense,
> imho.
>
> >
> > > This patch aims to
> > > add a solution to automatically create the default binder
> > > devices for each binderfs instance that gets mounted.
> > > To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
> > > the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
> > > are created in each binderfs instance instead of global devices
> > > being created by the binder driver.
> >
> > This is going to change how things work today, what is going to break
> > because of this change?
> >
> > I don't object to this, except for the worry of changing the default
> > behavior.
>
> This is something that Hridya and Todd can speak better to given that
> they suggested this change. :)
> From my perspective, binderfs binder devices and the regular binder
> driver are mostly used mutually exclusive in practice atm so that this
> change has little chance of breaking anyone.

As Christian says, we do not anticipate the change to break any
existing use cases since binderfs binder devices and regular binder
devices are generally not used simultaneously. Hopefully, there are
not a lot of unusual use cases since binderfs itself is relatively new
as well :)


>
> Now I really need to go back to vacation time - which I suck at
> apparently. :)
>
> Christian

Re: [PATCH v3 0/2] Add default binderfs devices

[Greg Kroah-Hartman]

On Thu, Aug 08, 2019 at 03:27:24PM -0700, Hridya Valsaraju wrote:
> Binderfs was created to help provide private binder devices to
> containers in their own IPC namespace. Currently, every time a new binderfs
> instance is mounted, its private binder devices need to be created via
> IOCTL calls. This patch series eliminates the effort for creating
> the default binder devices for each binderfs instance by creating them
> automatically.
> 
> Hridya Valsaraju (2):
>   binder: Add default binder devices through binderfs when configured
>   binder: Validate the default binderfs device names.

I'd like to get a reviewed-by from the other binder maintainers before
taking this series....

{hint}

Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Joel Fernandes]

On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote:
> Currently, since each binderfs instance needs its own
> private binder devices, every time a binderfs instance is
> mounted, all the default binder devices need to be created
> via the BINDER_CTL_ADD IOCTL. This patch aims to
> add a solution to automatically create the default binder
> devices for each binderfs instance that gets mounted.
> To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
> the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
> are created in each binderfs instance instead of global devices
> being created by the binder driver.
> 
> Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya@google.com>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> ---
> 
> Changes in v2:
> - Updated commit message as per Greg Kroah-Hartman.
> - Removed new module parameter creation as per Greg
>   Kroah-Hartman/Christian Brauner.
> - Refactored device name length check into a new patch as per Greg Kroah-Hartman.
> 
> Changes in v3:
> -Removed unnecessary empty lines as per Dan Carpenter.
> 
>  drivers/android/binder.c          |  5 +++--
>  drivers/android/binder_internal.h |  2 ++
>  drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 466b6a7f8ab7..ca6b21a53321 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
>  	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
>  module_param_named(debug_mask, binder_debug_mask, uint, 0644);
>  
> -static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
> +char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
>  module_param_named(devices, binder_devices_param, charp, 0444);
>  
>  static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
> @@ -6279,7 +6279,8 @@ static int __init binder_init(void)
>  				    &transaction_log_fops);
>  	}
>  
> -	if (strcmp(binder_devices_param, "") != 0) {
> +	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> +	    strcmp(binder_devices_param, "") != 0) {
>  		/*
>  		* Copy the module_parameter string, because we don't want to
>  		* tokenize it in-place.
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 045b3e42d98b..fe8c745dc8e0 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -37,6 +37,8 @@ struct binder_device {
>  
>  extern const struct file_operations binder_fops;
>  
> +extern char *binder_devices_param;
> +
>  #ifdef CONFIG_ANDROID_BINDERFS
>  extern bool is_binderfs_device(const struct inode *inode);
>  #else
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index e773f45d19d9..aee46dd1be91 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
>  	req->major = MAJOR(binderfs_dev);
>  	req->minor = minor;
>  
> -	ret = copy_to_user(userp, req, sizeof(*req));
> -	if (ret) {
> +	if (userp && copy_to_user(userp, req, sizeof(*req))) {
>  		ret = -EFAULT;
>  		goto err;
>  	}
> @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	int ret;
>  	struct binderfs_info *info;
>  	struct inode *inode = NULL;
> +	struct binderfs_device device_info = { 0 };
> +	const char *name;
> +	size_t len;
>  
>  	sb->s_blocksize = PAGE_SIZE;
>  	sb->s_blocksize_bits = PAGE_SHIFT;
> @@ -521,7 +523,22 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!sb->s_root)
>  		return -ENOMEM;
>  
> -	return binderfs_binder_ctl_create(sb);
> +	ret = binderfs_binder_ctl_create(sb);
> +	if (ret)
> +		return ret;
> +
> +	name = binder_devices_param;
> +	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> +		strscpy(device_info.name, name, len + 1);
> +		ret = binderfs_binder_device_create(inode, NULL, &device_info);
> +		if (ret)
> +			return ret;
> +		name += len;
> +		if (*name == ',')
> +			name++;
> +	}
> +
> +	return 0;
>  }
>  
>  static struct dentry *binderfs_mount(struct file_system_type *fs_type,
> -- 
> 2.22.0.770.g0f2c4a37fd-goog
> 

Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Joel Fernandes]

On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote:
> Currently, since each binderfs instance needs its own
> private binder devices, every time a binderfs instance is
> mounted, all the default binder devices need to be created
> via the BINDER_CTL_ADD IOCTL. This patch aims to
> add a solution to automatically create the default binder
> devices for each binderfs instance that gets mounted.
> To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
> the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
> are created in each binderfs instance instead of global devices
> being created by the binder driver.
> 
> Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya@google.com>
> ---

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
> Changes in v2:
> - Updated commit message as per Greg Kroah-Hartman.
> - Removed new module parameter creation as per Greg
>   Kroah-Hartman/Christian Brauner.
> - Refactored device name length check into a new patch as per Greg Kroah-Hartman.
> 
> Changes in v3:
> -Removed unnecessary empty lines as per Dan Carpenter.
> 
>  drivers/android/binder.c          |  5 +++--
>  drivers/android/binder_internal.h |  2 ++
>  drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 466b6a7f8ab7..ca6b21a53321 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
>  	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
>  module_param_named(debug_mask, binder_debug_mask, uint, 0644);
>  
> -static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
> +char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
>  module_param_named(devices, binder_devices_param, charp, 0444);
>  
>  static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
> @@ -6279,7 +6279,8 @@ static int __init binder_init(void)
>  				    &transaction_log_fops);
>  	}
>  
> -	if (strcmp(binder_devices_param, "") != 0) {
> +	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> +	    strcmp(binder_devices_param, "") != 0) {
>  		/*
>  		* Copy the module_parameter string, because we don't want to
>  		* tokenize it in-place.
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 045b3e42d98b..fe8c745dc8e0 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -37,6 +37,8 @@ struct binder_device {
>  
>  extern const struct file_operations binder_fops;
>  
> +extern char *binder_devices_param;
> +
>  #ifdef CONFIG_ANDROID_BINDERFS
>  extern bool is_binderfs_device(const struct inode *inode);
>  #else
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index e773f45d19d9..aee46dd1be91 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
>  	req->major = MAJOR(binderfs_dev);
>  	req->minor = minor;
>  
> -	ret = copy_to_user(userp, req, sizeof(*req));
> -	if (ret) {
> +	if (userp && copy_to_user(userp, req, sizeof(*req))) {
>  		ret = -EFAULT;
>  		goto err;
>  	}
> @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	int ret;
>  	struct binderfs_info *info;
>  	struct inode *inode = NULL;
> +	struct binderfs_device device_info = { 0 };
> +	const char *name;
> +	size_t len;
>  
>  	sb->s_blocksize = PAGE_SIZE;
>  	sb->s_blocksize_bits = PAGE_SHIFT;
> @@ -521,7 +523,22 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!sb->s_root)
>  		return -ENOMEM;
>  
> -	return binderfs_binder_ctl_create(sb);
> +	ret = binderfs_binder_ctl_create(sb);
> +	if (ret)
> +		return ret;
> +
> +	name = binder_devices_param;
> +	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> +		strscpy(device_info.name, name, len + 1);
> +		ret = binderfs_binder_device_create(inode, NULL, &device_info);
> +		if (ret)
> +			return ret;
> +		name += len;
> +		if (*name == ',')
> +			name++;
> +	}
> +
> +	return 0;
>  }
>  
>  static struct dentry *binderfs_mount(struct file_system_type *fs_type,
> -- 
> 2.22.0.770.g0f2c4a37fd-goog
> 

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

[Joel Fernandes]

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@ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya@google.com>
> ---

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

>  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. */
> +	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++;
> +	}
>  
>  	/* Allocate new major number for binderfs. */
>  	ret = alloc_chrdev_region(&binderfs_dev, 0, BINDERFS_MAX_MINOR,
> -- 
> 2.22.0.770.g0f2c4a37fd-goog
> 

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

[Greg Kroah-Hartman]

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@ubuntu.com> 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@ubuntu.com>
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > > ---
> > > >  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?

thanks,

greg k-h

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@ubuntu.com> 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@ubuntu.com>
> > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > > > ---
> > > > >  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

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

[Greg Kroah-Hartman]

On Wed, Sep 04, 2019 at 12:44:32PM +0200, Christian Brauner wrote:
> 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@ubuntu.com> 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@ubuntu.com>
> > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > > > > ---
> > > > > >  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?

Patches in email is fine, but can someone resend this one as I no longer
have this series in my queue anymore?

thanks,

greg k-h

[RESEND PATCH v3 0/2] Add default binderfs devices

[Christian Brauner]

Hey,

This is a resend of Hridya's series to add default binderfs devices. No
semantical changes were made. Only Joel's Acks were added by me.

Binderfs was created to help provide private binder devices to
containers in their own IPC namespace. Currently, every time a new binderfs
instance is mounted, its private binder devices need to be created via
IOCTL calls. This patch series eliminates the effort for creating
the default binder devices for each binderfs instance by creating them
automatically.

/* v0 */
Link: https://lore.kernel.org/lkml/20190801223556.209184-1-hridya@google.com

/* v1 */
Link: https://lore.kernel.org/lkml/20190806184007.60739-1-hridya@google.com

/* v2 */
Link: https://lore.kernel.org/lkml/20190808222727.132744-1-hridya@google.com/

Thanks!
Christian

Hridya Valsaraju (2):
  binder: Add default binder devices through binderfs when configured
  binder: Validate the default binderfs device names.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 35 ++++++++++++++++++++++++++++---
 3 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.23.0

[RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured

[Christian Brauner]

From: Hridya Valsaraju <hridya@google.com>

Currently, since each binderfs instance needs its own
private binder devices, every time a binderfs instance is
mounted, all the default binder devices need to be created
via the BINDER_CTL_ADD IOCTL. This patch aims to
add a solution to automatically create the default binder
devices for each binderfs instance that gets mounted.
To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
are created in each binderfs instance instead of global devices
being created by the binder driver.

Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20190808222727.132744-2-hridya@google.com
---
 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index dc1c83eafc22..ef2d3e582368 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -122,7 +122,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
 	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
 module_param_named(debug_mask, binder_debug_mask, uint, 0644);
 
-static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
 module_param_named(devices, binder_devices_param, charp, 0444);
 
 static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
@@ -6131,7 +6131,8 @@ static int __init binder_init(void)
 				    &transaction_log_fops);
 	}
 
-	if (strcmp(binder_devices_param, "") != 0) {
+	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+	    strcmp(binder_devices_param, "") != 0) {
 		/*
 		* Copy the module_parameter string, because we don't want to
 		* tokenize it in-place.
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 045b3e42d98b..fe8c745dc8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -37,6 +37,8 @@ struct binder_device {
 
 extern const struct file_operations binder_fops;
 
+extern char *binder_devices_param;
+
 #ifdef CONFIG_ANDROID_BINDERFS
 extern bool is_binderfs_device(const struct inode *inode);
 #else
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..aee46dd1be91 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
 	req->major = MAJOR(binderfs_dev);
 	req->minor = minor;
 
-	ret = copy_to_user(userp, req, sizeof(*req));
-	if (ret) {
+	if (userp && copy_to_user(userp, req, sizeof(*req))) {
 		ret = -EFAULT;
 		goto err;
 	}
@@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	int ret;
 	struct binderfs_info *info;
 	struct inode *inode = NULL;
+	struct binderfs_device device_info = { 0 };
+	const char *name;
+	size_t len;
 
 	sb->s_blocksize = PAGE_SIZE;
 	sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +523,22 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	if (!sb->s_root)
 		return -ENOMEM;
 
-	return binderfs_binder_ctl_create(sb);
+	ret = binderfs_binder_ctl_create(sb);
+	if (ret)
+		return ret;
+
+	name = binder_devices_param;
+	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+		strscpy(device_info.name, name, len + 1);
+		ret = binderfs_binder_device_create(inode, NULL, &device_info);
+		if (ret)
+			return ret;
+		name += len;
+		if (*name == ',')
+			name++;
+	}
+
+	return 0;
 }
 
 static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-- 
2.23.0

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

[Christian Brauner]

From: Hridya Valsaraju <hridya@google.com>

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@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20190808222727.132744-3-hridya@google.com
---
 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. */
+	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++;
+	}
 
 	/* Allocate new major number for binderfs. */
 	ret = alloc_chrdev_region(&binderfs_dev, 0, BINDERFS_MAX_MINOR,
-- 
2.23.0

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

[Christian Brauner]

On Wed, Sep 04, 2019 at 12:49:39PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2019 at 12:44:32PM +0200, Christian Brauner wrote:
> > 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@ubuntu.com> 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@ubuntu.com>
> > > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > > > > > ---
> > > > > > >  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?
> 
> Patches in email is fine, but can someone resend this one as I no longer
> have this series in my queue anymore?

Done!
Christian

Re: [RESEND PATCH v3 0/2] Add default binderfs devices

[Greg KH]

On Wed, Sep 04, 2019 at 01:07:02PM +0200, Christian Brauner wrote:
> Hey,
> 
> This is a resend of Hridya's series to add default binderfs devices. No
> semantical changes were made. Only Joel's Acks were added by me.
> 
> Binderfs was created to help provide private binder devices to
> containers in their own IPC namespace. Currently, every time a new binderfs
> instance is mounted, its private binder devices need to be created via
> IOCTL calls. This patch series eliminates the effort for creating
> the default binder devices for each binderfs instance by creating them
> automatically.

All now applied, thanks!

greg k-h