On Thu, Oct 26, 2023 at 01:47:32AM +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. > > This whole process of 1) determining smaller length so we don't overread > the buffer and 2) manually NUL-terminating our buffer so we can use in > string APIs is handled implicitly by strscpy(). > > Therefore, a suitable replacement is `strscpy` [2] due to the fact that > it guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding. > > 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(). > This pattern of 1) allocating memory for a string, 2) setting all bytes > to NUL, 3) copy bytes from another string + ensure NUL-padded > destination is just an open-coded kmemdup_nul(). > > The last case involves swapping kmalloc_array() for kcalloc() to give us > a zero-filled two-element array for both old_driver_version and > driver_version without needing the memset from init_driver_version(). > > Now this code is easier to read and less fragile (no more ... - 1's) or > min length checks and now we have guaranteed NUL-termination everywhere! > > Although perhaps there should be a macro for: > > 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@xxxxxxxxxxxxxxx > Cc: Kees Cook <keescook@xxxxxxxxxxxx> > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/scsi/hpsa.c | 29 +++++++++++------------------ > 1 file changed, 11 insertions(+), 18 deletions(-) > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > index af18d20f3079..3376d4614fe5 100644 > --- a/drivers/scsi/hpsa.c > +++ b/drivers/scsi/hpsa.c > @@ -452,16 +452,15 @@ 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'; > + strscpy(tmpbuf, buf, count); This is wrong -- "count" isn't the size of tmpbuf -- it's the size of the source, i.e. strlen(buf). > + > if (sscanf(tmpbuf, "%d", &status) != 1) > return -EINVAL; And this is immediately using the tmpbuf for getting an int. All of this should be replaced by kstrtoint(). > h = shost_to_hba(shost); > @@ -476,16 +475,15 @@ 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'; > + strscpy(tmpbuf, buf, count); > + > if (sscanf(tmpbuf, "%d", &debug_level) != 1) > return -EINVAL; Same thing here. > if (debug_level < 0) > @@ -7234,24 +7232,19 @@ 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); > > - driver_version = kmalloc(size, GFP_KERNEL); > + driver_version = kmemdup_nul(HPSA " " HPSA_DRIVER_VERSION, size, > + GFP_KERNEL); "size" isn't the length of the string here, so this results in an over-read from the .data segment: drivers/scsi/hpsa.c:#define HPSA "hpsa" drivers/scsi/hpsa.c:#define HPSA_DRIVER_VERSION "3.4.20-200" strlen(HSPA " " HPSA_DRIVER_VERSION) == 15 (16 with %NUL terminator) sizeof(cfgtable->driver_version) == 32: struct CfgTable { ... u8 driver_version[32]; > 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]); And then this will write garbage out to the driver for the 16 bytes following the string... :( Also, this thing is doing an alloc/free for a tiny string. That can just be on the stack: char driver_version[sizeof(cfgtable->driver_version)] = HPSA " " HPSA_DRIVER_VERSION; No alloc/free, no strscpy, easy easy. (Since the string is explicitly sized, the remaining space will be zero-initialized.) > + > kfree(driver_version); > return 0; > } > @@ -7271,7 +7264,7 @@ 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); > + old_driver_ver = kcalloc(2, size, GFP_KERNEL); > if (!old_driver_ver) > return -ENOMEM; > driver_ver = old_driver_ver + size; > @@ -7279,7 +7272,7 @@ static int controller_reset_failed(struct CfgTable __iomem *cfgtable) > /* 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); > + strscpy(old_driver_ver, HPSA " " HPSA_DRIVER_VERSION, size); > read_driver_ver_from_cfgtable(cfgtable, driver_ver); > rc = !memcmp(driver_ver, old_driver_ver, size); > kfree(old_driver_ver); This function is also wild -- it's allocating 2 strings (but at the same time, and using offsets to get to them), and again -- why? Just use the stack for 64 bytes: char driver_ver[sizeof(cfgtable->driver_version)] = ""; char old_driver_ver[sizeof(cfgtable->driver_version)] = HPSA " " HPSA_DRIVER_VERSION; -Kees -- Kees Cook
