linux-kernel - Re: [PATCH] scsi: hpsa: Use min() to simplify code

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: <7a39a882-4f00-483e-942d-36a7cff53954@acm.org>
Date: Wed, 12 Feb 2025 10:48:41 -0800
From: Bart Van Assche <bvanassche@....org>
To: Thorsten Blum <thorsten.blum@...ux.dev>,
 Don Brace <don.brace@...rochip.com>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 "Martin K. Petersen" <martin.petersen@...cle.com>
Cc: storagedev@...rochip.com, linux-scsi@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: hpsa: Use min() to simplify code

On 2/12/25 3:55 AM, Thorsten Blum wrote:
> Use min() to simplify the host_store_hp_ssd_smart_path_status() and
> host_store_raid_offload_debug() functions.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> ---
>   drivers/scsi/hpsa.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 84d8de07b7ae..1d19eb2ca1d3 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -460,7 +460,7 @@ 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;
> +	len = min(count, sizeof(tmpbuf) - 1);
>   	strncpy(tmpbuf, buf, len);
>   	tmpbuf[len] = '\0';
>   	if (sscanf(tmpbuf, "%d", &status) != 1)
> @@ -484,7 +484,7 @@ 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;
> +	len = min(count, sizeof(tmpbuf) - 1);
>   	strncpy(tmpbuf, buf, len);
>   	tmpbuf[len] = '\0';
>   	if (sscanf(tmpbuf, "%d", &debug_level) != 1)

 From Documentation/process/deprecated.rst:

<quote>
strncpy() on NUL-terminated strings
-----------------------------------
Use of strncpy() does not guarantee that the destination buffer will
be NUL terminated. This can lead to various linear read overflows and
other misbehavior due to the missing termination. It also NUL-pads
the destination buffer if the source contents are shorter than the
destination buffer size, which may be a needless performance penalty
for callers using only NUL-terminated strings.

When the destination is required to be NUL-terminated, the replacement is
strscpy(), though care must be given to any cases where the return value
of strncpy() was used, since strscpy() does not return a pointer to the
destination, but rather a count of non-NUL bytes copied (or negative
errno when it truncates). Any cases still needing NUL-padding should
instead use strscpy_pad().

If a caller is using non-NUL-terminated strings, strtomem() should be
used, and the destinations should be marked with the `__nonstring
<https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
attribute to avoid future compiler warnings. For cases still needing
NUL-padding, strtomem_pad() can be used.
</quote>

Instead of only changing the calculation of 'len', please change the
strncpy() calls into strscpy() calls.

Thanks,

Bart.