diff mbox series

[v2] scsi: hpsa: replace deprecated strncpy

Message ID 20231026-strncpy-drivers-scsi-hpsa-c-v2-1-2fe2d05122fd@google.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] scsi: hpsa: replace deprecated strncpy | expand

Commit Message

Justin Stitt Oct. 26, 2023, 11:13 p.m. UTC
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

Instances of strncpy()'ing a string into a buffer and manually
NUL-terminating followed by sccanf with just "%d" as the format
specifier can be accomplished by strscpy() and kstrtoint().

strscpy() guarantees NUL-termination on the destination buffer and
kstrtoint is better way of getting strings turned into ints.

For the last two strncpy() use cases in init_driver_version(), we can
actually drop this function entirely.

Firstly, we are kmalloc()'ing driver_version. Then, we are calling
init_driver_version() which memset's it to 0 followed by a strncpy().
The pattern is 1) allocating memory for a string, 2) setting all bytes
to NUL, 3) copy bytes from another string + ensure NUL-padded.

For these, we can just stack allocate driver_version and
old_driver_version. This simplifies the code greatly as we don't have
any malloc/free or strncpy's.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- use stack for buffers (thanks Kees)
- use kstrtoint (thanks Kees)
- Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/scsi/hpsa.c | 53 ++++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)


---
base-commit: d88520ad73b79e71e3ddf08de335b8520ae41c5c
change-id: 20231026-strncpy-drivers-scsi-hpsa-c-4cb7bd4e9b7f

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Oct. 27, 2023, 4:04 p.m. UTC | #1
On Thu, Oct 26, 2023 at 11:13:41PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> Instances of strncpy()'ing a string into a buffer and manually
> NUL-terminating followed by sccanf with just "%d" as the format
> specifier can be accomplished by strscpy() and kstrtoint().
> 
> strscpy() guarantees NUL-termination on the destination buffer and
> kstrtoint is better way of getting strings turned into ints.
> 
> For the last two strncpy() use cases in init_driver_version(), we can
> actually drop this function entirely.
> 
> Firstly, we are kmalloc()'ing driver_version. Then, we are calling
> init_driver_version() which memset's it to 0 followed by a strncpy().
> The pattern is 1) allocating memory for a string, 2) setting all bytes
> to NUL, 3) copy bytes from another string + ensure NUL-padded.
> 
> For these, we can just stack allocate driver_version and
> old_driver_version. This simplifies the code greatly as we don't have
> any malloc/free or strncpy's.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Changes in v2:
> - use stack for buffers (thanks Kees)
> - use kstrtoint (thanks Kees)
> - Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com
> ---
> Note: build-tested only.
> 
> Found with: $ rg "strncpy\("
> ---
>  drivers/scsi/hpsa.c | 53 ++++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index af18d20f3079..4d42fbb071cf 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -452,18 +452,18 @@ static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
>  					 struct device_attribute *attr,
>  					 const char *buf, size_t count)
>  {
> -	int status, len;
> +	int status;
>  	struct ctlr_info *h;
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  	char tmpbuf[10];
>  
>  	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
>  		return -EACCES;
> -	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> -	strncpy(tmpbuf, buf, len);
> -	tmpbuf[len] = '\0';
> -	if (sscanf(tmpbuf, "%d", &status) != 1)
> +
> +	strscpy(tmpbuf, buf, sizeof(tmpbuf));
> +	if (kstrtoint(tmpbuf, 0, &status))

I actually meant:

	if (kstrtoint(buf, 0, &status))

I don't see any reason for "tmpbuf" at all.

> @@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> -static void init_driver_version(char *driver_version, int len)
> -{
> -	memset(driver_version, 0, len);
> -	strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1);
> -}
> -
>  static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
>  {
> -	char *driver_version;
>  	int i, size = sizeof(cfgtable->driver_version);
> +	char driver_version[sizeof(cfgtable->driver_version)] =
> +						HPSA " " HPSA_DRIVER_VERSION;
>  
> -	driver_version = kmalloc(size, GFP_KERNEL);
> -	if (!driver_version)
> -		return -ENOMEM;
> -
> -	init_driver_version(driver_version, size);
>  	for (i = 0; i < size; i++)
>  		writeb(driver_version[i], &cfgtable->driver_version[i]);
> -	kfree(driver_version);
> +
>  	return 0;
>  }
>  
> @@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable,
>  static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
>  {
>  
> -	char *driver_ver, *old_driver_ver;
> -	int rc, size = sizeof(cfgtable->driver_version);
> -
> -	old_driver_ver = kmalloc_array(2, size, GFP_KERNEL);
> -	if (!old_driver_ver)
> -		return -ENOMEM;
> -	driver_ver = old_driver_ver + size;
> +	char driver_ver[sizeof(cfgtable->driver_version)] = "";
> +	char old_driver_ver[sizeof(cfgtable->driver_version)] =
> +						HPSA " " HPSA_DRIVER_VERSION;
> +	int rc;
>  
>  	/* After a reset, the 32 bytes of "driver version" in the cfgtable
>  	 * should have been changed, otherwise we know the reset failed.
>  	 */
> -	init_driver_version(old_driver_ver, size);
>  	read_driver_ver_from_cfgtable(cfgtable, driver_ver);
> -	rc = !memcmp(driver_ver, old_driver_ver, size);
> -	kfree(old_driver_ver);
> +	rc = !memcmp(driver_ver, old_driver_ver,
> +		     sizeof(cfgtable->driver_version));
> +
>  	return rc;
>  }
>  /* This does a hard reset of the controller using PCI power management

These two look good now; thanks!

-Kees
Justin Stitt Oct. 27, 2023, 8 p.m. UTC | #2
On Fri, Oct 27, 2023 at 9:04 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 26, 2023 at 11:13:41PM +0000, Justin Stitt wrote:
> > strncpy() is deprecated for use on NUL-terminated destination strings
> > [1] and as such we should prefer more robust and less ambiguous string
> > interfaces.
> >
> > Instances of strncpy()'ing a string into a buffer and manually
> > NUL-terminating followed by sccanf with just "%d" as the format
> > specifier can be accomplished by strscpy() and kstrtoint().
> >
> > strscpy() guarantees NUL-termination on the destination buffer and
> > kstrtoint is better way of getting strings turned into ints.
> >
> > For the last two strncpy() use cases in init_driver_version(), we can
> > actually drop this function entirely.
> >
> > Firstly, we are kmalloc()'ing driver_version. Then, we are calling
> > init_driver_version() which memset's it to 0 followed by a strncpy().
> > The pattern is 1) allocating memory for a string, 2) setting all bytes
> > to NUL, 3) copy bytes from another string + ensure NUL-padded.
> >
> > For these, we can just stack allocate driver_version and
> > old_driver_version. This simplifies the code greatly as we don't have
> > any malloc/free or strncpy's.
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> > Changes in v2:
> > - use stack for buffers (thanks Kees)
> > - use kstrtoint (thanks Kees)
> > - Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com
> > ---
> > Note: build-tested only.
> >
> > Found with: $ rg "strncpy\("
> > ---
> >  drivers/scsi/hpsa.c | 53 ++++++++++++++++++++---------------------------------
> >  1 file changed, 20 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index af18d20f3079..4d42fbb071cf 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -452,18 +452,18 @@ static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
> >                                        struct device_attribute *attr,
> >                                        const char *buf, size_t count)
> >  {
> > -     int status, len;
> > +     int status;
> >       struct ctlr_info *h;
> >       struct Scsi_Host *shost = class_to_shost(dev);
> >       char tmpbuf[10];
> >
> >       if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> >               return -EACCES;
> > -     len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> > -     strncpy(tmpbuf, buf, len);
> > -     tmpbuf[len] = '\0';
> > -     if (sscanf(tmpbuf, "%d", &status) != 1)
> > +
> > +     strscpy(tmpbuf, buf, sizeof(tmpbuf));
> > +     if (kstrtoint(tmpbuf, 0, &status))
>
> I actually meant:
>
>         if (kstrtoint(buf, 0, &status))

How do we know `buf` is NUL-terminated as kstrtoint() demands:

/**
 * kstrtoint - convert a string to an int
 * @s: The start of the string. The string must be null-terminated, and may also
 *  include a single newline before its terminating null. The first character
 *  may also be a plus sign or a minus sign.
...

>
> I don't see any reason for "tmpbuf" at all.
>
> > @@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,
> >       return 0;
> >  }
> >
> > -static void init_driver_version(char *driver_version, int len)
> > -{
> > -     memset(driver_version, 0, len);
> > -     strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1);
> > -}
> > -
> >  static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
> >  {
> > -     char *driver_version;
> >       int i, size = sizeof(cfgtable->driver_version);
> > +     char driver_version[sizeof(cfgtable->driver_version)] =
> > +                                             HPSA " " HPSA_DRIVER_VERSION;
> >
> > -     driver_version = kmalloc(size, GFP_KERNEL);
> > -     if (!driver_version)
> > -             return -ENOMEM;
> > -
> > -     init_driver_version(driver_version, size);
> >       for (i = 0; i < size; i++)
> >               writeb(driver_version[i], &cfgtable->driver_version[i]);
> > -     kfree(driver_version);
> > +
> >       return 0;
> >  }
> >
> > @@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable,
> >  static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
> >  {
> >
> > -     char *driver_ver, *old_driver_ver;
> > -     int rc, size = sizeof(cfgtable->driver_version);
> > -
> > -     old_driver_ver = kmalloc_array(2, size, GFP_KERNEL);
> > -     if (!old_driver_ver)
> > -             return -ENOMEM;
> > -     driver_ver = old_driver_ver + size;
> > +     char driver_ver[sizeof(cfgtable->driver_version)] = "";
> > +     char old_driver_ver[sizeof(cfgtable->driver_version)] =
> > +                                             HPSA " " HPSA_DRIVER_VERSION;
> > +     int rc;
> >
> >       /* After a reset, the 32 bytes of "driver version" in the cfgtable
> >        * should have been changed, otherwise we know the reset failed.
> >        */
> > -     init_driver_version(old_driver_ver, size);
> >       read_driver_ver_from_cfgtable(cfgtable, driver_ver);
> > -     rc = !memcmp(driver_ver, old_driver_ver, size);
> > -     kfree(old_driver_ver);
> > +     rc = !memcmp(driver_ver, old_driver_ver,
> > +                  sizeof(cfgtable->driver_version));
> > +
> >       return rc;
> >  }
> >  /* This does a hard reset of the controller using PCI power management
>
> These two look good now; thanks!

Woot!

>
> -Kees
>
> --
> Kees Cook

Thanks
Justin
diff mbox series

Patch

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index af18d20f3079..4d42fbb071cf 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -452,18 +452,18 @@  static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf, size_t count)
 {
-	int status, len;
+	int status;
 	struct ctlr_info *h;
 	struct Scsi_Host *shost = class_to_shost(dev);
 	char tmpbuf[10];
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		return -EACCES;
-	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
-	strncpy(tmpbuf, buf, len);
-	tmpbuf[len] = '\0';
-	if (sscanf(tmpbuf, "%d", &status) != 1)
+
+	strscpy(tmpbuf, buf, sizeof(tmpbuf));
+	if (kstrtoint(tmpbuf, 0, &status))
 		return -EINVAL;
+
 	h = shost_to_hba(shost);
 	h->acciopath_status = !!status;
 	dev_warn(&h->pdev->dev,
@@ -476,18 +476,18 @@  static ssize_t host_store_raid_offload_debug(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf, size_t count)
 {
-	int debug_level, len;
+	int debug_level;
 	struct ctlr_info *h;
 	struct Scsi_Host *shost = class_to_shost(dev);
 	char tmpbuf[10];
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		return -EACCES;
-	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
-	strncpy(tmpbuf, buf, len);
-	tmpbuf[len] = '\0';
-	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
+
+	strscpy(tmpbuf, buf, sizeof(tmpbuf));
+	if (kstrtoint(tmpbuf, 0, &debug_level))
 		return -EINVAL;
+
 	if (debug_level < 0)
 		debug_level = 0;
 	h = shost_to_hba(shost);
@@ -7234,25 +7234,15 @@  static int hpsa_controller_hard_reset(struct pci_dev *pdev,
 	return 0;
 }
 
-static void init_driver_version(char *driver_version, int len)
-{
-	memset(driver_version, 0, len);
-	strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1);
-}
-
 static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
 {
-	char *driver_version;
 	int i, size = sizeof(cfgtable->driver_version);
+	char driver_version[sizeof(cfgtable->driver_version)] =
+						HPSA " " HPSA_DRIVER_VERSION;
 
-	driver_version = kmalloc(size, GFP_KERNEL);
-	if (!driver_version)
-		return -ENOMEM;
-
-	init_driver_version(driver_version, size);
 	for (i = 0; i < size; i++)
 		writeb(driver_version[i], &cfgtable->driver_version[i]);
-	kfree(driver_version);
+
 	return 0;
 }
 
@@ -7268,21 +7258,18 @@  static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable,
 static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
 {
 
-	char *driver_ver, *old_driver_ver;
-	int rc, size = sizeof(cfgtable->driver_version);
-
-	old_driver_ver = kmalloc_array(2, size, GFP_KERNEL);
-	if (!old_driver_ver)
-		return -ENOMEM;
-	driver_ver = old_driver_ver + size;
+	char driver_ver[sizeof(cfgtable->driver_version)] = "";
+	char old_driver_ver[sizeof(cfgtable->driver_version)] =
+						HPSA " " HPSA_DRIVER_VERSION;
+	int rc;
 
 	/* After a reset, the 32 bytes of "driver version" in the cfgtable
 	 * should have been changed, otherwise we know the reset failed.
 	 */
-	init_driver_version(old_driver_ver, size);
 	read_driver_ver_from_cfgtable(cfgtable, driver_ver);
-	rc = !memcmp(driver_ver, old_driver_ver, size);
-	kfree(old_driver_ver);
+	rc = !memcmp(driver_ver, old_driver_ver,
+		     sizeof(cfgtable->driver_version));
+
 	return rc;
 }
 /* This does a hard reset of the controller using PCI power management