linux-kernel - Re: [PATCH 1/4] binder: add a mount option to show global stats

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190828092237.GA23192@kroah.com>
Date:   Wed, 28 Aug 2019 11:22:37 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Hridya Valsaraju <hridya@...gle.com>
Cc:     Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <christian@...uner.io>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        kernel-team@...roid.com
Subject: Re: [PATCH 1/4] binder: add a mount option to show global stats

On Tue, Aug 27, 2019 at 01:41:49PM -0700, Hridya Valsaraju wrote:
> Currently, all binder state and statistics live in debugfs.
> We need this information even when debugfs is not mounted.
> This patch adds the mount option 'stats' to enable a binderfs
> instance to have binder debug information present in the same.
> 'stats=global' will enable the global binder statistics. In
> the future, 'stats=local' will enable binder statistics local
> to the binderfs instance. The two modes 'global' and 'local'
> will be mutually exclusive. 'stats=global' option is only available
> for a binderfs instance mounted in the initial user namespace.
> An attempt to use the option to mount a binderfs instance in
> another user namespace will return an EPERM error.
> 
> Signed-off-by: Hridya Valsaraju <hridya@...gle.com>
> ---
>  drivers/android/binderfs.c | 47 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index cc2e71576396..d95d179aec58 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -51,18 +51,27 @@ static DEFINE_IDA(binderfs_minors);
>  /**
>   * binderfs_mount_opts - mount options for binderfs
>   * @max: maximum number of allocatable binderfs binder devices
> + * @stats_mode: enable binder stats in binderfs.
>   */
>  struct binderfs_mount_opts {
>  	int max;
> +	int stats_mode;
>  };
>  
>  enum {
>  	Opt_max,
> +	Opt_stats_mode,
>  	Opt_err
>  };
>  
> +enum binderfs_stats_mode {
> +	STATS_NONE,
> +	STATS_GLOBAL,
> +};
> +
>  static const match_table_t tokens = {
>  	{ Opt_max, "max=%d" },
> +	{ Opt_stats_mode, "stats=%s" },
>  	{ Opt_err, NULL     }
>  };
>  
> @@ -290,8 +299,9 @@ static void binderfs_evict_inode(struct inode *inode)
>  static int binderfs_parse_mount_opts(char *data,
>  				     struct binderfs_mount_opts *opts)
>  {
> -	char *p;
> +	char *p, *stats;
>  	opts->max = BINDERFS_MAX_MINOR;
> +	opts->stats_mode = STATS_NONE;
>  
>  	while ((p = strsep(&data, ",")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
> @@ -311,6 +321,24 @@ static int binderfs_parse_mount_opts(char *data,
>  
>  			opts->max = max_devices;
>  			break;
> +		case Opt_stats_mode:
> +			stats = match_strdup(&args[0]);
> +			if (!stats)
> +				return -ENOMEM;
> +
> +			if (strcmp(stats, "global") != 0) {
> +				kfree(stats);
> +				return -EINVAL;
> +			}
> +
> +			if (!capable(CAP_SYS_ADMIN)) {
> +				kfree(stats);
> +				return -EINVAL;

Can a non-CAP_SYS_ADMIN task even call this function?  Anyway, if it
can, put the check at the top of the case, and just return early before
doing any extra work like checking values or allocating memory.

> +			}
> +
> +			opts->stats_mode = STATS_GLOBAL;
> +			kfree(stats);
> +			break;
>  		default:
>  			pr_err("Invalid mount options\n");
>  			return -EINVAL;
> @@ -322,8 +350,21 @@ static int binderfs_parse_mount_opts(char *data,
>  
>  static int binderfs_remount(struct super_block *sb, int *flags, char *data)
>  {
> +	int prev_stats_mode, ret;
>  	struct binderfs_info *info = sb->s_fs_info;
> -	return binderfs_parse_mount_opts(data, &info->mount_opts);
> +
> +	prev_stats_mode = info->mount_opts.stats_mode;
> +	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> +	if (ret)
> +		return ret;
> +
> +	if (prev_stats_mode != info->mount_opts.stats_mode) {
> +		pr_info("Binderfs stats mode cannot be changed during a remount\n");

pr_err()?

thanks,

greg k-h