linux-kernel - Re: [PATCH v2] scsi: hpsa: Replace deprecated strncpy() with strscpy()

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202502131231.98A662655@keescook>
Date: Thu, 13 Feb 2025 12:35:15 -0800
From: Kees Cook <kees@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Don Brace <don.brace@...rochip.com>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-hardening@...r.kernel.org,
	Bart Van Assche <bvanassche@....org>, storagedev@...rochip.com,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] scsi: hpsa: Replace deprecated strncpy() with
 strscpy()

On Thu, Feb 13, 2025 at 08:53:33PM +0100, Thorsten Blum wrote:
> strncpy() is deprecated for NUL-terminated destination buffers. Use
> strscpy() instead and remove the manual NUL-termination.
> 
> Use min() to simplify the size calculation.
> 
> Compile-tested only.
> 
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@...r.kernel.org
> Suggested-by: Bart Van Assche <bvanassche@....org>
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> ---
> Changes in v2:
> - Adjust len to copy the same number of bytes as with strncpy()
> - Link to v1: https://lore.kernel.org/r/34BB4FDE-062D-4C1B-B246-86CB55F631B8@linux.dev/
> ---
>  drivers/scsi/hpsa.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 84d8de07b7ae..c7ebae24b09f 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -460,9 +460,8 @@ static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,

for added context:

        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';
> +	len = min(count + 1, sizeof(tmpbuf));
> +	strscpy(tmpbuf, buf, len);
>  	if (sscanf(tmpbuf, "%d", &status) != 1)
>  		return -EINVAL;
>  	h = shost_to_hba(shost);

Okay, this is making sure that "len" is either the last byte of tmpbuf,
or all the bytes in buf, if it is less than sizeof(tmpbuf), and then NUL
terminates the string that was copied into tmpbuf from buf.

This is what memtostr() was made for.

> @@ -484,9 +483,8 @@ static ssize_t host_store_raid_offload_debug(struct device *dev,
>  
>  	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';
> +	len = min(count + 1, sizeof(tmpbuf));
> +	strscpy(tmpbuf, buf, len);
>  	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
>  		return -EINVAL;
>  	if (debug_level < 0)

I think this patch should be:

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 84d8de07b7ae..bff0e768b042 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -453,16 +453,14 @@ 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';
+	memtostr(tmpbuf, buf);
 	if (sscanf(tmpbuf, "%d", &status) != 1)
 		return -EINVAL;
 	h = shost_to_hba(shost);
@@ -477,16 +475,14 @@ 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';
+	memtostr(tmpbuf, buf);
 	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
 		return -EINVAL;
 	if (debug_level < 0)

-- 
Kees Cook