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

From: Joel Granados
Date: Mon Sep 04 2023 - 09:11:57 EST

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@xxxxxxxxxxxxxxx>
> ---
> 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
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: signature.asc
Description: PGP signature