| Message ID | 20250212222214.86110-2-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: hpsa: Replace deprecated strncpy() with strscpy() | expand |
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
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.
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)
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(-)