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

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

[Thorsten Blum]

strncpy() is deprecated for NUL-terminated destination buffers [1]. Use
strscpy() instead and remove the manual NUL-termination.

Use min() to simplify the size calculation.

Compile-tested only.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Suggested-by: Bart Van Assche <bvanassche@acm.org>
---
 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..9399e101f150 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, sizeof(tmpbuf) - 1);
+	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, sizeof(tmpbuf) - 1);
+	strscpy(tmpbuf, buf, len);
 	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
 		return -EINVAL;
 	if (debug_level < 0)
-- 
2.48.1

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

[Thorsten Blum]

On 12. Feb 2025, at 23:22, Thorsten Blum wrote:
> strncpy() is deprecated for NUL-terminated destination buffers [1]. Use
> strscpy() instead and remove the manual NUL-termination.
> 
> Use min() to simplify the size calculation.
> 
> Compile-tested only.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 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..9399e101f150 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, sizeof(tmpbuf) - 1);
> +	strscpy(tmpbuf, buf, len);

With strscpy() it should probably just be sizeof(tmpbuf) without -1, and
then add +1 to count for the number of copied bytes to be the same as
with strncpy().

Like this:

	len = min(count + 1, sizeof(tmpbuf));

This subtle difference between strncpy() and strscpy() regarding the
number of bytes copied isn't really documented anywhere, is it? The
documentation I came across so far seems to focus mostly on the
different return values of the two functions.

> 	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, sizeof(tmpbuf) - 1);
> +	strscpy(tmpbuf, buf, len);

Same here.

> 	if (sscanf(tmpbuf, "%d", &debug_level) != 1)
> 		return -EINVAL;
> 	if (debug_level < 0)

Maybe someone can confirm my reasoning before I submit a v2?

Thanks,
Thorsten

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

[Bart Van Assche]

On 2/13/25 3:24 AM, Thorsten Blum wrote:
> This subtle difference between strncpy() and strscpy() regarding the
> number of bytes copied isn't really documented anywhere, is it? The
> documentation I came across so far seems to focus mostly on the
> different return values of the two functions.

 From the description of commit 9022ed0e7e65 ("strscpy: write destination
buffer only once"):

     So strscpy not only guarantees NUL-termination (unlike strncpy), it 
also
     doesn't do unnecessary padding at the destination.  But at the same 
time
     also avoids byte-at-a-time reads and writes by _allowing_ some 
extra NUL
     writes - within the size, of course - so that the whole copy can be 
done
     with word operations.

     It is also stable in the face of a mutable source string: it explicitly
     does not read the source buffer multiple times (so an implementation
     using "strnlen()+memcpy()" would be wrong), and does not read the 
source
     buffer past the size (like the mis-design that is strlcpy does).

     Finally, the return value is designed to be simple and unambiguous: if
     the string cannot be copied fully, it returns an actual negative error,
     making error handling clearer and simpler (and the caller already knows
     the size of the buffer).  Otherwise it returns the string length of the
     result.

More information is available in the description of commit 30c44659f4a3
("Merge branch 'strscpy' of 
git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile").

Thanks,

Bart.