linux-kernel - Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting

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]

Date:   Mon, 4 Sep 2023 15:11:45 +0200
From:   Joel Granados <j.granados@...sung.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     Luis Chamberlain <mcgrof@...nel.org>,
        <linux-kernel@...r.kernel.org>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>
Subject: Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer
 formatting

On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote:
> Improve readability and maintainability by replacing a hardcoded string
> allocation and formatting by the use of the kasprintf() helper.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/parport/procfs.c | 53 +++++++---------------------------------
>  drivers/parport/share.c  | 15 +++++-------
>  include/linux/parport.h  |  2 --
>  3 files changed, 15 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 4e5b972c3e26..7aa99c65b934 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -32,13 +32,6 @@
>  #define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ)
>  #define PARPORT_MIN_SPINTIME_VALUE 1
>  #define PARPORT_MAX_SPINTIME_VALUE 1000
> -/*
> - * PARPORT_BASE_* is the size of the known parts of the sysctl path
> - * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9
> - * and null char(1).
> - */
> -#define PARPORT_BASE_PATH_SIZE 13
> -#define PARPORT_BASE_DEVICES_PATH_SIZE 22
>  
>  static int do_active_device(struct ctl_table *table, int write,
>  		      void *result, size_t *lenp, loff_t *ppos)
> @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port)
>  {
>  	struct parport_sysctl_table *t;
>  	char *tmp_dir_path;
> -	size_t tmp_path_len, port_name_len;
> -	int bytes_written, i, err = 0;
> +	int i, err = 0;
>  
>  	t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
>  	if (t == NULL)
> @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port)
For this function I would even go a step further and start with the two
kasprintf calls so we can then free them in the reverse order. And then
leave the rest as it is. I attached an untested diff that applies on
top of your changes to show you what I mean.

>  		t->vars[5 + i].extra2 = &port->probe_info[i];
>  	}
>  
> -	port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> -	/*
> -	 * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> -	 * We calculate for the second as that will give us enough for the first.
> -	 */
> -	tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> -	tmp_dir_path = kzalloc(tmp_path_len, GFP_KERNEL);
> +	tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name);
>  	if (!tmp_dir_path) {
>  		err = -ENOMEM;
>  		goto exit_free_t;
>  	}
>  
> -	bytes_written = snprintf(tmp_dir_path, tmp_path_len,
> -				 "dev/parport/%s/devices", port->name);
> -	if (tmp_path_len <= bytes_written) {
> -		err = -ENOENT;
> -		goto exit_free_tmp_dir_path;
> -	}
>  	t->devices_header = register_sysctl(tmp_dir_path, t->device_dir);
>  	if (t->devices_header == NULL) {
>  		err = -ENOENT;
>  		goto  exit_free_tmp_dir_path;
>  	}
>  
> -	tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len;
> -	bytes_written = snprintf(tmp_dir_path, tmp_path_len,
> -				 "dev/parport/%s", port->name);
> -	if (tmp_path_len <= bytes_written) {
> -		err = -ENOENT;
> +	kfree(tmp_dir_path);
> +
> +	tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name);
> +	if (!tmp_dir_path) {
> +		err = -ENOMEM;
>  		goto unregister_devices_h;
>  	}
>  
> @@ -514,34 +494,22 @@ int parport_proc_unregister(struct parport *port)
>  
>  int parport_device_proc_register(struct pardevice *device)
>  {
> -	int bytes_written, err = 0;
>  	struct parport_device_sysctl_table *t;
>  	struct parport * port = device->port;
> -	size_t port_name_len, device_name_len, tmp_dir_path_len;
>  	char *tmp_dir_path;

...

> diff --git a/include/linux/parport.h b/include/linux/parport.h
> index 999eddd619b7..fff39bc30629 100644
> --- a/include/linux/parport.h
> +++ b/include/linux/parport.h
> @@ -180,8 +180,6 @@ struct ieee1284_info {
>  	struct semaphore irq;
>  };
>  
> -#define PARPORT_NAME_MAX_LEN 15
This variable protected against port->name not ending in '\0'. Anyone
worried that kasprintf could be unbounded?

> -
>  /* A parallel port */
>  struct parport {
>  	unsigned long base;	/* base address */
> -- 
> 2.40.0.1.gaa8946217a0b
> 

-- 

Joel Granados

View attachment "parport.patch" of type "text/x-diff" (2226 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)