diff mbox series

scsi: hpsa: replace deprecated strncpy with strscpy/kmemdup_nul

Message ID 20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com (mailing list archive)
State Changes Requested
Headers show
Series scsi: hpsa: replace deprecated strncpy with strscpy/kmemdup_nul | expand

Commit Message

Justin Stitt Oct. 26, 2023, 1:47 a.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.

This whole process of 1) determining smaller length so we don't overread
the buffer and 2) manually NUL-terminating our buffer so we can use in
string APIs is handled implicitly by strscpy().

Therefore, a suitable replacement is `strscpy` [2] due to the fact that
it guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.

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().
This pattern of 1) allocating memory for a string, 2) setting all bytes
to NUL, 3) copy bytes from another string + ensure NUL-padded
destination is just an open-coded kmemdup_nul().

The last case involves swapping kmalloc_array() for kcalloc() to give us
a zero-filled two-element array for both old_driver_version and
driver_version without needing the memset from init_driver_version().

Now this code is easier to read and less fragile (no more ... - 1's) or
min length checks and now we have guaranteed NUL-termination everywhere!

Although perhaps there should be a macro for:

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>
---
Note: build-tested only.

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


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

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

Comments

Kees Cook Oct. 26, 2023, 5:44 p.m. UTC | #1 
On Thu, Oct 26, 2023 at 01:47:32AM +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.
> 
> This whole process of 1) determining smaller length so we don't overread
> the buffer and 2) manually NUL-terminating our buffer so we can use in
> string APIs is handled implicitly by strscpy().
> 
> Therefore, a suitable replacement is `strscpy` [2] due to the fact that
> it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> 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().
> This pattern of 1) allocating memory for a string, 2) setting all bytes
> to NUL, 3) copy bytes from another string + ensure NUL-padded
> destination is just an open-coded kmemdup_nul().
> 
> The last case involves swapping kmalloc_array() for kcalloc() to give us
> a zero-filled two-element array for both old_driver_version and
> driver_version without needing the memset from init_driver_version().
> 
> Now this code is easier to read and less fragile (no more ... - 1's) or
> min length checks and now we have guaranteed NUL-termination everywhere!
> 
> Although perhaps there should be a macro for:
> 
> 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>
> ---
> Note: build-tested only.
> 
> Found with: $ rg "strncpy\("
> ---
>  drivers/scsi/hpsa.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index af18d20f3079..3376d4614fe5 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -452,16 +452,15 @@ 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';
> +	strscpy(tmpbuf, buf, count);

This is wrong -- "count" isn't the size of tmpbuf -- it's the size of
the source, i.e.  strlen(buf).

> +
>  	if (sscanf(tmpbuf, "%d", &status) != 1)
>  		return -EINVAL;

And this is immediately using the tmpbuf for getting an int. All of this
should be replaced by kstrtoint().

>  	h = shost_to_hba(shost);
> @@ -476,16 +475,15 @@ 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';
> +	strscpy(tmpbuf, buf, count);
> +
>  	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
>  		return -EINVAL;

Same thing here.

>  	if (debug_level < 0)
> @@ -7234,24 +7232,19 @@ 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);
>  
> -	driver_version = kmalloc(size, GFP_KERNEL);
> +	driver_version = kmemdup_nul(HPSA " " HPSA_DRIVER_VERSION, size,
> +				     GFP_KERNEL);

"size" isn't the length of the string here, so this results in an
over-read from the .data segment:

drivers/scsi/hpsa.c:#define HPSA "hpsa"
drivers/scsi/hpsa.c:#define HPSA_DRIVER_VERSION "3.4.20-200"

strlen(HSPA " " HPSA_DRIVER_VERSION) == 15 (16 with %NUL terminator)

sizeof(cfgtable->driver_version) == 32:

struct CfgTable {
	...
        u8              driver_version[32];

>  	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]);

And then this will write garbage out to the driver for the 16 bytes
following the string... :(

Also, this thing is doing an alloc/free for a tiny string. That can just
be on the stack:

	char driver_version[sizeof(cfgtable->driver_version)] = HPSA " " HPSA_DRIVER_VERSION;

No alloc/free, no strscpy, easy easy. (Since the string is explicitly
sized, the remaining space will be zero-initialized.)


> +
>  	kfree(driver_version);
>  	return 0;
>  }
> @@ -7271,7 +7264,7 @@ 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);
> +	old_driver_ver = kcalloc(2, size, GFP_KERNEL);
>  	if (!old_driver_ver)
>  		return -ENOMEM;
>  	driver_ver = old_driver_ver + size;
> @@ -7279,7 +7272,7 @@ static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
>  	/* 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);
> +	strscpy(old_driver_ver, HPSA " " HPSA_DRIVER_VERSION, size);
>  	read_driver_ver_from_cfgtable(cfgtable, driver_ver);
>  	rc = !memcmp(driver_ver, old_driver_ver, size);
>  	kfree(old_driver_ver);

This function is also wild -- it's allocating 2 strings (but at the same
time, and using offsets to get to them), and again -- why? Just use the
stack for 64 bytes:

	char driver_ver[sizeof(cfgtable->driver_version)] = "";
	char old_driver_ver[sizeof(cfgtable->driver_version)] = HPSA " " HPSA_DRIVER_VERSION;

-Kees
diff mbox series

Patch

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index af18d20f3079..3376d4614fe5 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -452,16 +452,15 @@  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';
+	strscpy(tmpbuf, buf, count);
+
 	if (sscanf(tmpbuf, "%d", &status) != 1)
 		return -EINVAL;
 	h = shost_to_hba(shost);
@@ -476,16 +475,15 @@  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';
+	strscpy(tmpbuf, buf, count);
+
 	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
 		return -EINVAL;
 	if (debug_level < 0)
@@ -7234,24 +7232,19 @@  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);
 
-	driver_version = kmalloc(size, GFP_KERNEL);
+	driver_version = kmemdup_nul(HPSA " " HPSA_DRIVER_VERSION, 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;
 }
@@ -7271,7 +7264,7 @@  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);
+	old_driver_ver = kcalloc(2, size, GFP_KERNEL);
 	if (!old_driver_ver)
 		return -ENOMEM;
 	driver_ver = old_driver_ver + size;
@@ -7279,7 +7272,7 @@  static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
 	/* 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);
+	strscpy(old_driver_ver, HPSA " " HPSA_DRIVER_VERSION, size);
 	read_driver_ver_from_cfgtable(cfgtable, driver_ver);
 	rc = !memcmp(driver_ver, old_driver_ver, size);
 	kfree(old_driver_ver);