| Message ID | 20250213195332.1464-3-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] scsi: hpsa: Replace deprecated strncpy() with strscpy() | expand |
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.
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)
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. :)
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. >
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)
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(-)