| Message ID | 20231026-strncpy-drivers-scsi-hpsa-c-v2-1-2fe2d05122fd@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] scsi: hpsa: replace deprecated strncpy | expand |
On Thu, Oct 26, 2023 at 11:13:41PM +0000, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > Instances of strncpy()'ing a string into a buffer and manually > NUL-terminating followed by sccanf with just "%d" as the format > specifier can be accomplished by strscpy() and kstrtoint(). > > strscpy() guarantees NUL-termination on the destination buffer and > kstrtoint is better way of getting strings turned into ints. > > For the last two strncpy() use cases in init_driver_version(), we can > actually drop this function entirely. > > Firstly, we are kmalloc()'ing driver_version. Then, we are calling > init_driver_version() which memset's it to 0 followed by a strncpy(). > The pattern is 1) allocating memory for a string, 2) setting all bytes > to NUL, 3) copy bytes from another string + ensure NUL-padded. > > For these, we can just stack allocate driver_version and > old_driver_version. This simplifies the code greatly as we don't have > any malloc/free or strncpy's. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Changes in v2: > - use stack for buffers (thanks Kees) > - use kstrtoint (thanks Kees) > - Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/scsi/hpsa.c | 53 ++++++++++++++++++++--------------------------------- > 1 file changed, 20 insertions(+), 33 deletions(-) > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > index af18d20f3079..4d42fbb071cf 100644 > --- a/drivers/scsi/hpsa.c > +++ b/drivers/scsi/hpsa.c > @@ -452,18 +452,18 @@ 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'; > - if (sscanf(tmpbuf, "%d", &status) != 1) > + > + strscpy(tmpbuf, buf, sizeof(tmpbuf)); > + if (kstrtoint(tmpbuf, 0, &status)) I actually meant: if (kstrtoint(buf, 0, &status)) I don't see any reason for "tmpbuf" at all. > @@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev, > return 0; > } > > -static void init_driver_version(char *driver_version, int len) > -{ > - memset(driver_version, 0, len); > - strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1); > -} > - > static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable) > { > - char *driver_version; > int i, size = sizeof(cfgtable->driver_version); > + char driver_version[sizeof(cfgtable->driver_version)] = > + HPSA " " HPSA_DRIVER_VERSION; > > - driver_version = kmalloc(size, GFP_KERNEL); > - if (!driver_version) > - return -ENOMEM; > - > - init_driver_version(driver_version, size); > for (i = 0; i < size; i++) > writeb(driver_version[i], &cfgtable->driver_version[i]); > - kfree(driver_version); > + > return 0; > } > > @@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable, > static int controller_reset_failed(struct CfgTable __iomem *cfgtable) > { > > - char *driver_ver, *old_driver_ver; > - int rc, size = sizeof(cfgtable->driver_version); > - > - old_driver_ver = kmalloc_array(2, size, GFP_KERNEL); > - if (!old_driver_ver) > - return -ENOMEM; > - driver_ver = old_driver_ver + size; > + char driver_ver[sizeof(cfgtable->driver_version)] = ""; > + char old_driver_ver[sizeof(cfgtable->driver_version)] = > + HPSA " " HPSA_DRIVER_VERSION; > + int rc; > > /* After a reset, the 32 bytes of "driver version" in the cfgtable > * should have been changed, otherwise we know the reset failed. > */ > - init_driver_version(old_driver_ver, size); > read_driver_ver_from_cfgtable(cfgtable, driver_ver); > - rc = !memcmp(driver_ver, old_driver_ver, size); > - kfree(old_driver_ver); > + rc = !memcmp(driver_ver, old_driver_ver, > + sizeof(cfgtable->driver_version)); > + > return rc; > } > /* This does a hard reset of the controller using PCI power management These two look good now; thanks! -Kees
On Fri, Oct 27, 2023 at 9:04 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 26, 2023 at 11:13:41PM +0000, Justin Stitt wrote: > > strncpy() is deprecated for use on NUL-terminated destination strings > > [1] and as such we should prefer more robust and less ambiguous string > > interfaces. > > > > Instances of strncpy()'ing a string into a buffer and manually > > NUL-terminating followed by sccanf with just "%d" as the format > > specifier can be accomplished by strscpy() and kstrtoint(). > > > > strscpy() guarantees NUL-termination on the destination buffer and > > kstrtoint is better way of getting strings turned into ints. > > > > For the last two strncpy() use cases in init_driver_version(), we can > > actually drop this function entirely. > > > > Firstly, we are kmalloc()'ing driver_version. Then, we are calling > > init_driver_version() which memset's it to 0 followed by a strncpy(). > > The pattern is 1) allocating memory for a string, 2) setting all bytes > > to NUL, 3) copy bytes from another string + ensure NUL-padded. > > > > For these, we can just stack allocate driver_version and > > old_driver_version. This simplifies the code greatly as we don't have > > any malloc/free or strncpy's. > > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-hardening@vger.kernel.org > > Cc: Kees Cook <keescook@chromium.org> > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > Changes in v2: > > - use stack for buffers (thanks Kees) > > - use kstrtoint (thanks Kees) > > - Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com > > --- > > Note: build-tested only. > > > > Found with: $ rg "strncpy\(" > > --- > > drivers/scsi/hpsa.c | 53 ++++++++++++++++++++--------------------------------- > > 1 file changed, 20 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > > index af18d20f3079..4d42fbb071cf 100644 > > --- a/drivers/scsi/hpsa.c > > +++ b/drivers/scsi/hpsa.c > > @@ -452,18 +452,18 @@ 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'; > > - if (sscanf(tmpbuf, "%d", &status) != 1) > > + > > + strscpy(tmpbuf, buf, sizeof(tmpbuf)); > > + if (kstrtoint(tmpbuf, 0, &status)) > > I actually meant: > > if (kstrtoint(buf, 0, &status)) How do we know `buf` is NUL-terminated as kstrtoint() demands: /** * kstrtoint - convert a string to an int * @s: The start of the string. The string must be null-terminated, and may also * include a single newline before its terminating null. The first character * may also be a plus sign or a minus sign. ... > > I don't see any reason for "tmpbuf" at all. > > > @@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev, > > return 0; > > } > > > > -static void init_driver_version(char *driver_version, int len) > > -{ > > - memset(driver_version, 0, len); > > - strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1); > > -} > > - > > static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable) > > { > > - char *driver_version; > > int i, size = sizeof(cfgtable->driver_version); > > + char driver_version[sizeof(cfgtable->driver_version)] = > > + HPSA " " HPSA_DRIVER_VERSION; > > > > - driver_version = kmalloc(size, GFP_KERNEL); > > - if (!driver_version) > > - return -ENOMEM; > > - > > - init_driver_version(driver_version, size); > > for (i = 0; i < size; i++) > > writeb(driver_version[i], &cfgtable->driver_version[i]); > > - kfree(driver_version); > > + > > return 0; > > } > > > > @@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable, > > static int controller_reset_failed(struct CfgTable __iomem *cfgtable) > > { > > > > - char *driver_ver, *old_driver_ver; > > - int rc, size = sizeof(cfgtable->driver_version); > > - > > - old_driver_ver = kmalloc_array(2, size, GFP_KERNEL); > > - if (!old_driver_ver) > > - return -ENOMEM; > > - driver_ver = old_driver_ver + size; > > + char driver_ver[sizeof(cfgtable->driver_version)] = ""; > > + char old_driver_ver[sizeof(cfgtable->driver_version)] = > > + HPSA " " HPSA_DRIVER_VERSION; > > + int rc; > > > > /* After a reset, the 32 bytes of "driver version" in the cfgtable > > * should have been changed, otherwise we know the reset failed. > > */ > > - init_driver_version(old_driver_ver, size); > > read_driver_ver_from_cfgtable(cfgtable, driver_ver); > > - rc = !memcmp(driver_ver, old_driver_ver, size); > > - kfree(old_driver_ver); > > + rc = !memcmp(driver_ver, old_driver_ver, > > + sizeof(cfgtable->driver_version)); > > + > > return rc; > > } > > /* This does a hard reset of the controller using PCI power management > > These two look good now; thanks! Woot! > > -Kees > > -- > Kees Cook Thanks Justin
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index af18d20f3079..4d42fbb071cf 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -452,18 +452,18 @@ 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'; - if (sscanf(tmpbuf, "%d", &status) != 1) + + strscpy(tmpbuf, buf, sizeof(tmpbuf)); + if (kstrtoint(tmpbuf, 0, &status)) return -EINVAL; + h = shost_to_hba(shost); h->acciopath_status = !!status; dev_warn(&h->pdev->dev, @@ -476,18 +476,18 @@ 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'; - if (sscanf(tmpbuf, "%d", &debug_level) != 1) + + strscpy(tmpbuf, buf, sizeof(tmpbuf)); + if (kstrtoint(tmpbuf, 0, &debug_level)) return -EINVAL; + if (debug_level < 0) debug_level = 0; h = shost_to_hba(shost); @@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev, return 0; } -static void init_driver_version(char *driver_version, int len) -{ - memset(driver_version, 0, len); - strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1); -} - static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable) { - char *driver_version; int i, size = sizeof(cfgtable->driver_version); + char driver_version[sizeof(cfgtable->driver_version)] = + HPSA " " HPSA_DRIVER_VERSION; - driver_version = kmalloc(size, GFP_KERNEL); - if (!driver_version) - return -ENOMEM; - - init_driver_version(driver_version, size); for (i = 0; i < size; i++) writeb(driver_version[i], &cfgtable->driver_version[i]); - kfree(driver_version); + return 0; } @@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct CfgTable __iomem *cfgtable, static int controller_reset_failed(struct CfgTable __iomem *cfgtable) { - char *driver_ver, *old_driver_ver; - int rc, size = sizeof(cfgtable->driver_version); - - old_driver_ver = kmalloc_array(2, size, GFP_KERNEL); - if (!old_driver_ver) - return -ENOMEM; - driver_ver = old_driver_ver + size; + char driver_ver[sizeof(cfgtable->driver_version)] = ""; + char old_driver_ver[sizeof(cfgtable->driver_version)] = + HPSA " " HPSA_DRIVER_VERSION; + int rc; /* After a reset, the 32 bytes of "driver version" in the cfgtable * should have been changed, otherwise we know the reset failed. */ - init_driver_version(old_driver_ver, size); read_driver_ver_from_cfgtable(cfgtable, driver_ver); - rc = !memcmp(driver_ver, old_driver_ver, size); - kfree(old_driver_ver); + rc = !memcmp(driver_ver, old_driver_ver, + sizeof(cfgtable->driver_version)); + return rc; } /* This does a hard reset of the controller using PCI power management
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Instances of strncpy()'ing a string into a buffer and manually NUL-terminating followed by sccanf with just "%d" as the format specifier can be accomplished by strscpy() and kstrtoint(). strscpy() guarantees NUL-termination on the destination buffer and kstrtoint is better way of getting strings turned into ints. For the last two strncpy() use cases in init_driver_version(), we can actually drop this function entirely. Firstly, we are kmalloc()'ing driver_version. Then, we are calling init_driver_version() which memset's it to 0 followed by a strncpy(). The pattern is 1) allocating memory for a string, 2) setting all bytes to NUL, 3) copy bytes from another string + ensure NUL-padded. For these, we can just stack allocate driver_version and old_driver_version. This simplifies the code greatly as we don't have any malloc/free or strncpy's. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v2: - use stack for buffers (thanks Kees) - use kstrtoint (thanks Kees) - Link to v1: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a191b@google.com --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/scsi/hpsa.c | 53 ++++++++++++++++++++--------------------------------- 1 file changed, 20 insertions(+), 33 deletions(-) --- base-commit: d88520ad73b79e71e3ddf08de335b8520ae41c5c change-id: 20231026-strncpy-drivers-scsi-hpsa-c-4cb7bd4e9b7f Best regards, -- Justin Stitt <justinstitt@google.com>