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

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4256 bytes --]

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@linux.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

[-- Attachment #1.2: parport.patch --]
[-- Type: text/x-diff, Size: 2313 bytes --]

diff --git i/drivers/parport/procfs.c w/drivers/parport/procfs.c
index 7aa99c65b934..0e3b01368aec 100644
--- i/drivers/parport/procfs.c
+++ w/drivers/parport/procfs.c
@@ -423,12 +423,24 @@ parport_default_sysctl_table = {
 int parport_proc_register(struct parport *port)
 {
 	struct parport_sysctl_table *t;
-	char *tmp_dir_path;
+	char *parport_name_devices, *parport_name;
 	int i, err = 0;
 
+	parport_name_devices = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name);
+	if (!parport_name_devices)
+		return -ENOMEM;
+
+	parport_name = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name);
+	if (!parport_name) {
+		err = -ENOMEM;
+		goto exit_partport_name_devices;
+	}
+
 	t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
-	if (t == NULL)
-		return -ENOMEM;
+	if (t == NULL) {
+		err = -ENOMEM;
+		goto exit_parport_name;
+	}
 
 	t->device_dir[0].extra1 = port;
 
@@ -438,27 +450,13 @@ int parport_proc_register(struct parport *port)
 		t->vars[5 + i].extra2 = &port->probe_info[i];
 	}
 
-	tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name);
-	if (!tmp_dir_path) {
-		err = -ENOMEM;
-		goto exit_free_t;
-	}
-
-	t->devices_header = register_sysctl(tmp_dir_path, t->device_dir);
+	t->devices_header = register_sysctl(parport_name_devices, t->device_dir);
 	if (t->devices_header == NULL) {
 		err = -ENOENT;
-		goto  exit_free_tmp_dir_path;
+		goto exit_free_t;
 	}
 
-	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;
-	}
-
-	t->port_header = register_sysctl(tmp_dir_path, t->vars);
+	t->port_header = register_sysctl(parport_name, t->vars);
 	if (t->port_header == NULL) {
 		err = -ENOENT;
 		goto unregister_devices_h;
@@ -466,17 +464,19 @@ int parport_proc_register(struct parport *port)
 
 	port->sysctl_table = t;
 
-	kfree(tmp_dir_path);
-	return 0;
+	goto exit_parport_name;
 
 unregister_devices_h:
 	unregister_sysctl_table(t->devices_header);
 
-exit_free_tmp_dir_path:
-	kfree(tmp_dir_path);
-
 exit_free_t:
 	kfree(t);
+
+exit_parport_name:
+	kfree(parport_name);
+
+exit_partport_name_devices:
+	kfree(parport_name_devices);
 	return err;
 }
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]