[PATCH v2] scsi: hpsa: Replace deprecated strncpy() with strscpy()

[PATCH v2] scsi: hpsa: Replace deprecated strncpy() with strscpy()

[Thorsten Blum]

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@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.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,
 
 	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);
@@ -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)
-- 
2.48.1

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

[Bart Van Assche]

On 2/13/25 11:53 AM, Thorsten Blum wrote:
> 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,
>   
>   	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);
> @@ -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)

Something I should have noticed earlier: this code occurs inside sysfs
write callbacks. The strings passed to sysfs write callbacks are
0-terminated. Hence, 'buf' can be passed directly to sscanf() and
tmpbuf[] can be removed. From kernfs_fop_write_iter() in fs/kernfs.c:

	buf[len] = '\0';	/* guarantee string termination */

Thanks,

Bart.

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

[Kees Cook]

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@vger.kernel.org
> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.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

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

[Kees Cook]

On Thu, Feb 13, 2025 at 12:34:55PM -0800, Bart Van Assche wrote:
> Something I should have noticed earlier: this code occurs inside sysfs
> write callbacks. The strings passed to sysfs write callbacks are
> 0-terminated. Hence, 'buf' can be passed directly to sscanf() and
> tmpbuf[] can be removed. From kernfs_fop_write_iter() in fs/kernfs.c:
> 
> 	buf[len] = '\0';	/* guarantee string termination */

Oh, good point! Yeah, ignore my last email. Yes: tmpbuf can be dropped
entirely. :)

-- 
Kees Cook

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

[David Laight]

On Thu, 13 Feb 2025 12:34:55 -0800
Bart Van Assche <bvanassche@acm.org> wrote:

> On 2/13/25 11:53 AM, Thorsten Blum wrote:
> > 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,
> >   
> >   	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);
> > @@ -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)  
> 
> Something I should have noticed earlier: this code occurs inside sysfs
> write callbacks. The strings passed to sysfs write callbacks are
> 0-terminated. Hence, 'buf' can be passed directly to sscanf() and
> tmpbuf[] can be removed. From kernfs_fop_write_iter() in fs/kernfs.c:
> 
> 	buf[len] = '\0';	/* guarantee string termination */

You might also want to use one of the stroul() family rather than sscanf().

	David.

> 
> Thanks,
> 
> Bart.
>