[PATCH 1/2] binderfs: respect limit on binder control creation

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/2] binderfs: respect limit on binder control creation
@ 2019-01-23 11:41 Christian Brauner
  2019-01-23 11:41 ` [PATCH 2/2] binderfs: remove separate device_initcall() Christian Brauner
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2019-01-23 11:41 UTC (permalink / raw)
  To: gregkh, tkjos, devel, linux-kernel
  Cc: arve, maco, joel, tkjos, Christian Brauner

We currently adhere to the reserved devices limit when creating new
binderfs devices in binderfs instances not located in the inital ipc
namespace. But it is still possible to rob the host instances of their 4
reserved devices by creating the maximum allowed number of devices in a
single binderfs instance located in a non-initial ipc namespace and then
mounting 4 separate binderfs instances in non-initial ipc namespaces. That
happens because the limit is currently not respected for the creation of
the initial binder-control device node. Block this nonsense by performing
the same check in binderfs_binder_ctl_create() that we perform in
binderfs_binder_device_create().

Fixes: 36bdf3cae09d ("binderfs: reserve devices for initial mount")
Signed-off-by: Christian Brauner <christian@brauner.io>
---
 drivers/android/binderfs.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 6a2185eb66c5..7a550104a722 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -395,6 +395,11 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 	struct inode *inode = NULL;
 	struct dentry *root = sb->s_root;
 	struct binderfs_info *info = sb->s_fs_info;
+#if defined(CONFIG_IPC_NS)
+	bool use_reserve = (info->ipc_ns == &init_ipc_ns);
+#else
+	bool use_reserve = true;
+#endif
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (!device)
@@ -413,7 +418,10 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 
 	/* Reserve a new minor number for the new device. */
 	mutex_lock(&binderfs_minors_mutex);
-	minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+	minor = ida_alloc_max(&binderfs_minors,
+			      use_reserve ? BINDERFS_MAX_MINOR :
+					    BINDERFS_MAX_MINOR_CAPPED,
+			      GFP_KERNEL);
 	mutex_unlock(&binderfs_minors_mutex);
 	if (minor < 0) {
 		ret = minor;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] binderfs: remove separate device_initcall()
  2019-01-23 11:41 [PATCH 1/2] binderfs: respect limit on binder control creation Christian Brauner
@ 2019-01-23 11:41 ` Christian Brauner
  2019-01-30 14:24   ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2019-01-23 11:41 UTC (permalink / raw)
  To: gregkh, tkjos, devel, linux-kernel
  Cc: arve, maco, joel, tkjos, Christian Brauner

binderfs should not have a separate device_initcall(). When a kernel is
compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
ANDROID_BINDER_DEVICES="".
When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
regression potential for legacy workloads.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 drivers/android/binder.c          | 4 ++++
 drivers/android/binder_internal.h | 9 +++++++++
 drivers/android/binderfs.c        | 4 +---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cdfc87629efb..751d76173f81 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5915,6 +5915,10 @@ static int __init binder_init(void)
 			goto err_init_binder_device_failed;
 	}
 
+	ret = init_binderfs();
+	if (ret)
+		goto err_init_binder_device_failed;
+
 	return ret;
 
 err_init_binder_device_failed:
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 7fb97f503ef2..045b3e42d98b 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode *inode)
 }
 #endif
 
+#ifdef CONFIG_ANDROID_BINDERFS
+extern int __init init_binderfs(void);
+#else
+static inline int __init init_binderfs(void)
+{
+	return 0;
+}
+#endif
+
 #endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7a550104a722..e773f45d19d9 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
 	.fs_flags	= FS_USERNS_MOUNT,
 };
 
-static int __init init_binderfs(void)
+int __init init_binderfs(void)
 {
 	int ret;
 
@@ -568,5 +568,3 @@ static int __init init_binderfs(void)
 
 	return ret;
 }
-
-device_initcall(init_binderfs);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] binderfs: remove separate device_initcall()
  2019-01-23 11:41 ` [PATCH 2/2] binderfs: remove separate device_initcall() Christian Brauner
@ 2019-01-30 14:24   ` Greg KH
  2019-01-30 17:01     ` Christian Brauner
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-01-30 14:24 UTC (permalink / raw)
  To: Christian Brauner; +Cc: tkjos, devel, linux-kernel, arve, maco, joel, tkjos

On Wed, Jan 23, 2019 at 12:41:16PM +0100, Christian Brauner wrote:
> binderfs should not have a separate device_initcall(). When a kernel is
> compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
> CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
> CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
> ANDROID_BINDER_DEVICES="".
> When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
> regression potential for legacy workloads.
> 
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
>  drivers/android/binder.c          | 4 ++++
>  drivers/android/binder_internal.h | 9 +++++++++
>  drivers/android/binderfs.c        | 4 +---
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cdfc87629efb..751d76173f81 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5915,6 +5915,10 @@ static int __init binder_init(void)
>  			goto err_init_binder_device_failed;
>  	}
>  
> +	ret = init_binderfs();
> +	if (ret)
> +		goto err_init_binder_device_failed;
> +
>  	return ret;
>  
>  err_init_binder_device_failed:
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 7fb97f503ef2..045b3e42d98b 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode *inode)
>  }
>  #endif
>  
> +#ifdef CONFIG_ANDROID_BINDERFS
> +extern int __init init_binderfs(void);
> +#else
> +static inline int __init init_binderfs(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif /* _LINUX_BINDER_INTERNAL_H */
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 7a550104a722..e773f45d19d9 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
>  	.fs_flags	= FS_USERNS_MOUNT,
>  };
>  
> -static int __init init_binderfs(void)
> +int __init init_binderfs(void)
>  {
>  	int ret;
>  
> @@ -568,5 +568,3 @@ static int __init init_binderfs(void)
>  
>  	return ret;
>  }
> -
> -device_initcall(init_binderfs);

I get a build warning when applying this patch :(

Please fix up and resend.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] binderfs: remove separate device_initcall()
  2019-01-30 14:24   ` Greg KH
@ 2019-01-30 17:01     ` Christian Brauner
  2019-01-30 21:17       ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2019-01-30 17:01 UTC (permalink / raw)
  To: Greg KH; +Cc: tkjos, devel, linux-kernel, arve, maco, joel, tkjos

On Wed, Jan 30, 2019 at 03:24:12PM +0100, Greg KH wrote:
> On Wed, Jan 23, 2019 at 12:41:16PM +0100, Christian Brauner wrote:
> > binderfs should not have a separate device_initcall(). When a kernel is
> > compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
> > CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
> > CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
> > ANDROID_BINDER_DEVICES="".
> > When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
> > regression potential for legacy workloads.
> > 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> >  drivers/android/binder.c          | 4 ++++
> >  drivers/android/binder_internal.h | 9 +++++++++
> >  drivers/android/binderfs.c        | 4 +---
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index cdfc87629efb..751d76173f81 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -5915,6 +5915,10 @@ static int __init binder_init(void)
> >  			goto err_init_binder_device_failed;
> >  	}
> >  
> > +	ret = init_binderfs();
> > +	if (ret)
> > +		goto err_init_binder_device_failed;
> > +
> >  	return ret;
> >  
> >  err_init_binder_device_failed:
> > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > index 7fb97f503ef2..045b3e42d98b 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode *inode)
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_ANDROID_BINDERFS
> > +extern int __init init_binderfs(void);
> > +#else
> > +static inline int __init init_binderfs(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 7a550104a722..e773f45d19d9 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
> >  	.fs_flags	= FS_USERNS_MOUNT,
> >  };
> >  
> > -static int __init init_binderfs(void)
> > +int __init init_binderfs(void)
> >  {
> >  	int ret;
> >  
> > @@ -568,5 +568,3 @@ static int __init init_binderfs(void)
> >  
> >  	return ret;
> >  }
> > -
> > -device_initcall(init_binderfs);
> 
> I get a build warning when applying this patch :(

Hm, I can't reproduce that build error with this patch applied to what
you currently have in char-misc-linus. :(
Any chance you can give me the config that produced this warning?
I tried with CONFIG_BINDERFS=y and CONFIG_BINDERFS=n.

Thanks!
Christian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] binderfs: remove separate device_initcall()
  2019-01-30 17:01     ` Christian Brauner
@ 2019-01-30 21:17       ` Greg KH
  2019-01-31  0:26         ` Christian Brauner
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-01-30 21:17 UTC (permalink / raw)
  To: Christian Brauner; +Cc: devel, tkjos, linux-kernel, arve, joel, maco, tkjos

On Wed, Jan 30, 2019 at 06:01:02PM +0100, Christian Brauner wrote:
> On Wed, Jan 30, 2019 at 03:24:12PM +0100, Greg KH wrote:
> > On Wed, Jan 23, 2019 at 12:41:16PM +0100, Christian Brauner wrote:
> > > binderfs should not have a separate device_initcall(). When a kernel is
> > > compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
> > > CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
> > > CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
> > > ANDROID_BINDER_DEVICES="".
> > > When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
> > > regression potential for legacy workloads.
> > > 
> > > Signed-off-by: Christian Brauner <christian@brauner.io>
> > > ---
> > >  drivers/android/binder.c          | 4 ++++
> > >  drivers/android/binder_internal.h | 9 +++++++++
> > >  drivers/android/binderfs.c        | 4 +---
> > >  3 files changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index cdfc87629efb..751d76173f81 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -5915,6 +5915,10 @@ static int __init binder_init(void)
> > >  			goto err_init_binder_device_failed;
> > >  	}
> > >  
> > > +	ret = init_binderfs();
> > > +	if (ret)
> > > +		goto err_init_binder_device_failed;
> > > +
> > >  	return ret;
> > >  
> > >  err_init_binder_device_failed:
> > > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > > index 7fb97f503ef2..045b3e42d98b 100644
> > > --- a/drivers/android/binder_internal.h
> > > +++ b/drivers/android/binder_internal.h
> > > @@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode *inode)
> > >  }
> > >  #endif
> > >  
> > > +#ifdef CONFIG_ANDROID_BINDERFS
> > > +extern int __init init_binderfs(void);
> > > +#else
> > > +static inline int __init init_binderfs(void)
> > > +{
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > >  #endif /* _LINUX_BINDER_INTERNAL_H */
> > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > index 7a550104a722..e773f45d19d9 100644
> > > --- a/drivers/android/binderfs.c
> > > +++ b/drivers/android/binderfs.c
> > > @@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
> > >  	.fs_flags	= FS_USERNS_MOUNT,
> > >  };
> > >  
> > > -static int __init init_binderfs(void)
> > > +int __init init_binderfs(void)
> > >  {
> > >  	int ret;
> > >  
> > > @@ -568,5 +568,3 @@ static int __init init_binderfs(void)
> > >  
> > >  	return ret;
> > >  }
> > > -
> > > -device_initcall(init_binderfs);
> > 
> > I get a build warning when applying this patch :(
> 
> Hm, I can't reproduce that build error with this patch applied to what
> you currently have in char-misc-linus. :(
> Any chance you can give me the config that produced this warning?
> I tried with CONFIG_BINDERFS=y and CONFIG_BINDERFS=n.

$ make M=drivers/android
  CC      drivers/android/binderfs.o
  CC      drivers/android/binder.o
drivers/android/binder.c: In function ‘binder_init’:
drivers/android/binder.c:5933:2: warning: ‘device_names’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  kfree(device_names);
  ^~~~~~~~~~~~~~~~~~~

$ gcc --version
gcc (GCC) 8.2.1 20181127

And gcc is right about this warning, that could happen with your change :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] binderfs: remove separate device_initcall()
  2019-01-30 21:17       ` Greg KH
@ 2019-01-31  0:26         ` Christian Brauner
  0 siblings, 0 replies; 6+ messages in thread
From: Christian Brauner @ 2019-01-31  0:26 UTC (permalink / raw)
  To: Greg KH; +Cc: devel, tkjos, linux-kernel, arve, joel, maco, tkjos

On Wed, Jan 30, 2019 at 10:17:39PM +0100, Greg KH wrote:
> On Wed, Jan 30, 2019 at 06:01:02PM +0100, Christian Brauner wrote:
> > On Wed, Jan 30, 2019 at 03:24:12PM +0100, Greg KH wrote:
> > > On Wed, Jan 23, 2019 at 12:41:16PM +0100, Christian Brauner wrote:
> > > > binderfs should not have a separate device_initcall(). When a kernel is
> > > > compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
> > > > CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
> > > > CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
> > > > ANDROID_BINDER_DEVICES="".
> > > > When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
> > > > regression potential for legacy workloads.
> > > > 
> > > > Signed-off-by: Christian Brauner <christian@brauner.io>
> > > > ---
> > > >  drivers/android/binder.c          | 4 ++++
> > > >  drivers/android/binder_internal.h | 9 +++++++++
> > > >  drivers/android/binderfs.c        | 4 +---
> > > >  3 files changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index cdfc87629efb..751d76173f81 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -5915,6 +5915,10 @@ static int __init binder_init(void)
> > > >  			goto err_init_binder_device_failed;
> > > >  	}
> > > >  
> > > > +	ret = init_binderfs();
> > > > +	if (ret)
> > > > +		goto err_init_binder_device_failed;
> > > > +
> > > >  	return ret;
> > > >  
> > > >  err_init_binder_device_failed:
> > > > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > > > index 7fb97f503ef2..045b3e42d98b 100644
> > > > --- a/drivers/android/binder_internal.h
> > > > +++ b/drivers/android/binder_internal.h
> > > > @@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode *inode)
> > > >  }
> > > >  #endif
> > > >  
> > > > +#ifdef CONFIG_ANDROID_BINDERFS
> > > > +extern int __init init_binderfs(void);
> > > > +#else
> > > > +static inline int __init init_binderfs(void)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > >  #endif /* _LINUX_BINDER_INTERNAL_H */
> > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > > index 7a550104a722..e773f45d19d9 100644
> > > > --- a/drivers/android/binderfs.c
> > > > +++ b/drivers/android/binderfs.c
> > > > @@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
> > > >  	.fs_flags	= FS_USERNS_MOUNT,
> > > >  };
> > > >  
> > > > -static int __init init_binderfs(void)
> > > > +int __init init_binderfs(void)
> > > >  {
> > > >  	int ret;
> > > >  
> > > > @@ -568,5 +568,3 @@ static int __init init_binderfs(void)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > -
> > > > -device_initcall(init_binderfs);
> > > 
> > > I get a build warning when applying this patch :(
> > 
> > Hm, I can't reproduce that build error with this patch applied to what
> > you currently have in char-misc-linus. :(
> > Any chance you can give me the config that produced this warning?
> > I tried with CONFIG_BINDERFS=y and CONFIG_BINDERFS=n.
> 
> $ make M=drivers/android
>   CC      drivers/android/binderfs.o
>   CC      drivers/android/binder.o
> drivers/android/binder.c: In function ‘binder_init’:
> drivers/android/binder.c:5933:2: warning: ‘device_names’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   kfree(device_names);
>   ^~~~~~~~~~~~~~~~~~~
> 
> $ gcc --version
> gcc (GCC) 8.2.1 20181127
> 
> And gcc is right about this warning, that could happen with your change :(

Thanks for the pointer. New version sent out that fixes this issue! :)

Thanks!
Christian

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-31  0:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-23 11:41 [PATCH 1/2] binderfs: respect limit on binder control creation Christian Brauner
2019-01-23 11:41 ` [PATCH 2/2] binderfs: remove separate device_initcall() Christian Brauner
2019-01-30 14:24   ` Greg KH
2019-01-30 17:01     ` Christian Brauner
2019-01-30 21:17       ` Greg KH
2019-01-31  0:26         ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox