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, 4 Sep 2023 15:11:45 +0200
From:   Joel Granados <j.granados@samsung.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     Luis Chamberlain <mcgrof@kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Subject: Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer
 formatting
Message-ID: <20230904131145.tp4umorb3t25tmsq@localhost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="c5hkprao7t3bkepg"
Content-Disposition: inline
In-Reply-To: <20230901134250.1172990-1-andriy.shevchenko@linux.intel.com>
CMS-TYPE: 201P
References: <CGME20230901134310eucas1p1d9be610c894d46f19bb6c12576aef94b@eucas1p1.samsung.com>
        <20230901134250.1172990-1-andriy.shevchenko@linux.intel.com>
Precedence: bulk

--c5hkprao7t3bkepg
Content-Type: multipart/mixed; boundary="gcfvjs5in7dsexwz"
Content-Disposition: inline

--gcfvjs5in7dsexwz
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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

=2E..

> 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;
>  };
> =20
> -#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 */
> --=20
> 2.40.0.1.gaa8946217a0b
>=20

--=20

Joel Granados

--gcfvjs5in7dsexwz
Content-Type: text/x-diff; charset="us-ascii"
Content-Disposition: attachment; filename="parport.patch"
Content-Transfer-Encoding: quoted-printable

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 =3D {
 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 =3D 0;
=20
+	parport_name_devices =3D kasprintf(GFP_KERNEL, "dev/parport/%s/devices", =
port->name);
+	if (!parport_name_devices)
+		return -ENOMEM;
+
+	parport_name =3D kasprintf(GFP_KERNEL, "dev/parport/%s", port->name);
+	if (!parport_name) {
+		err =3D -ENOMEM;
+		goto exit_partport_name_devices;
+	}
+
 	t =3D kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
-	if (t =3D=3D NULL)
-		return -ENOMEM;
+	if (t =3D=3D NULL) {
+		err =3D -ENOMEM;
+		goto exit_parport_name;
+	}
=20
 	t->device_dir[0].extra1 =3D port;
=20
@@ -438,27 +450,13 @@ int parport_proc_register(struct parport *port)
 		t->vars[5 + i].extra2 =3D &port->probe_info[i];
 	}
=20
-	tmp_dir_path =3D kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->na=
me);
-	if (!tmp_dir_path) {
-		err =3D -ENOMEM;
-		goto exit_free_t;
-	}
-
-	t->devices_header =3D register_sysctl(tmp_dir_path, t->device_dir);
+	t->devices_header =3D register_sysctl(parport_name_devices, t->device_dir=
);
 	if (t->devices_header =3D=3D NULL) {
 		err =3D -ENOENT;
-		goto  exit_free_tmp_dir_path;
+		goto exit_free_t;
 	}
=20
-	kfree(tmp_dir_path);
-
-	tmp_dir_path =3D kasprintf(GFP_KERNEL, "dev/parport/%s", port->name);
-	if (!tmp_dir_path) {
-		err =3D -ENOMEM;
-		goto unregister_devices_h;
-	}
-
-	t->port_header =3D register_sysctl(tmp_dir_path, t->vars);
+	t->port_header =3D register_sysctl(parport_name, t->vars);
 	if (t->port_header =3D=3D NULL) {
 		err =3D -ENOENT;
 		goto unregister_devices_h;
@@ -466,17 +464,19 @@ int parport_proc_register(struct parport *port)
=20
 	port->sysctl_table =3D t;
=20
-	kfree(tmp_dir_path);
-	return 0;
+	goto exit_parport_name;
=20
 unregister_devices_h:
 	unregister_sysctl_table(t->devices_header);
=20
-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;
 }
=20

--gcfvjs5in7dsexwz--

--c5hkprao7t3bkepg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmT113wACgkQupfNUreW
QU9s2wv+M4/gR2ggBzJQcRG2gwOrNhZSNWpY5cw9mNhH8eUNbrY3M+wTlXyJ+ywc
SXKzrf2doVS4OjUnBi3gstugGI+A7cIxqX5iBk0YyHqU92hTqi/bEcKMRV7KjA+k
d1v1+jEUEqw6pJ4t0hXAY2EH9eCIHnCzrOr0qcyr9V6HrhhbXGsO+Mcwr65oIkxp
j4TwmSUrAlqDrXOOeKhA22gXj1op/cssKCP2x/ftMsCctlBo6w4zuXVXerPevINy
Kyfbf1BuWkU/jtdv1Z7WRj4DiuFppxN2xXgP2l/6IyfyFxdh/wB6sq81zFtBs4ls
RQcI8Y3PQ+mBpcezumQidlOSZ4otCRpQF+L4EcAcK7w3A7foOpisLlYP2j+/t9lv
f1ObDnHENmQFQQiQbu8vu33MkufNIWtXEx3/LvH/2tG2aPe8gGFbcDQfmpTWNNQm
7+16AhB7WOnv//2TtAYw268X7Mq6c5Rsj0oTtqV7y3TnpkQXY5PJeg6OQB72FvJj
k6O4rH+m
=/4Of
-----END PGP SIGNATURE-----

--c5hkprao7t3bkepg--