Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 23 May 2022 12:33:53 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        willy@infradead.org, Luis Chamberlain <mcgrof@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>
Subject: Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool
Message-ID: <202205231233.EE3AB926@keescook>
References: <20220522052624.21493-1-songmuchun@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220522052624.21493-1-songmuchun@bytedance.com>
Precedence: bulk

On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> robustness, fix it by reimplementing proc_dobool() properly.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Iurii Zaikin <yzaikin@google.com>
> ---
> v2:
>  - Reimplementing proc_dobool().
> 
>  fs/lockd/svc.c  |  2 +-
>  kernel/sysctl.c | 38 +++++++++++++++++++-------------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 59ef8a1f843f..6e48ee787f49 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -496,7 +496,7 @@ static struct ctl_table nlm_sysctls[] = {
>  	{
>  		.procname	= "nsm_use_hostnames",
>  		.data		= &nsm_use_hostnames,
> -		.maxlen		= sizeof(int),
> +		.maxlen		= sizeof(nsm_use_hostnames),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dobool,
>  	},

This hunk is fine -- it's a reasonable fix-up.

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e52b6e372c60..50a2c29efc94 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -423,21 +423,6 @@ static void proc_put_char(void **buf, size_t *size, char c)
>  	}
>  }
>  
> -static int do_proc_dobool_conv(bool *negp, unsigned long *lvalp,
> -				int *valp,
> -				int write, void *data)
> -{
> -	if (write) {
> -		*(bool *)valp = *lvalp;
> -	} else {
> -		int val = *(bool *)valp;
> -
> -		*lvalp = (unsigned long)val;
> -		*negp = false;
> -	}
> -	return 0;
> -}
> -
>  static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>  				 int *valp,
>  				 int write, void *data)
> @@ -708,16 +693,31 @@ int do_proc_douintvec(struct ctl_table *table, int write,
>   * @lenp: the size of the user buffer
>   * @ppos: file position
>   *
> - * Reads/writes up to table->maxlen/sizeof(unsigned int) integer
> - * values from/to the user buffer, treated as an ASCII string.
> + * Reads/writes up to table->maxlen/sizeof(bool) bool values from/to
> + * the user buffer, treated as an ASCII string.
>   *
>   * Returns 0 on success.
>   */
>  int proc_dobool(struct ctl_table *table, int write, void *buffer,
>  		size_t *lenp, loff_t *ppos)
>  {
> -	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> -				do_proc_dobool_conv, NULL);
> +	struct ctl_table tmp = *table;
> +	bool *data = table->data;
> +	unsigned int val = READ_ONCE(*data);
> +	int ret;
> +
> +	/* Do not support arrays yet. */
> +	if (table->maxlen != sizeof(bool))
> +		return -EINVAL;
> +
> +	tmp.maxlen = sizeof(val);
> +	tmp.data = &val;
> +	ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL);
> +	if (ret)
> +		return ret;
> +	if (write)
> +		WRITE_ONCE(*data, val ? true : false);
> +	return 0;
>  }

This part I don't understand -- it just inlines do_proc_dobool_conv(),
and I think detracts from readability.

-- 
Kees Cook