| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFhGd8ooYaPBXyWLTKD94-jiR0Na4qnyRT602R1tzjdF9W3BWg@mail.gmail.com>
Date: Fri, 27 Oct 2023 13:00:25 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Kees Cook <keescook@...omium.org>
Cc: Don Brace <don.brace@...rochip.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
storagedev@...rochip.com, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] scsi: hpsa: replace deprecated strncpy
On Fri, Oct 27, 2023 at 9:04 AM Kees Cook <keescook@...omium.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@...r.kernel.org
> > Cc: Kees Cook <keescook@...omium.org>
> > Signed-off-by: Justin Stitt <justinstitt@...gle.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
Powered by blists - more mailing lists