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: [email protected]
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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-75519d7a1...@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
--
Kees Cook
