lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 26 Oct 2023 10:44:28 -0700
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Don Brace <don.brace@...rochip.com>,
	"James E.J. Bottomley" <jejb@...ux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	storagedev@...rochip.com, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] scsi: hpsa: replace deprecated strncpy with
 strscpy/kmemdup_nul

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@...r.kernel.org
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Justin Stitt <justinstitt@...gle.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

-- 
Kees Cook

Powered by blists - more mailing lists