linux-kernel - Re: [PATCH] binderfs: implement sysctls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181221135509.GA30679@kroah.com>
Date:   Fri, 21 Dec 2018 14:55:09 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Christian Brauner <christian@...uner.io>
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: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
are putting them in the "global" namespace, that feels like something
that binderfs was trying to avoid in the first place.

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

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