linux-kernel - Re: [PATCH] wifi: ipw2x00: replace deprecated strncpy with strscpy

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAFhGd8rrCgSa8BMOSxRFeto6w=Vn-SMdVU4Dg5zfSSLe5HBZ4w@mail.gmail.com>
Date:   Tue, 1 Aug 2023 17:54:54 -0700
From:   Justin Stitt <justinstitt@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Stanislav Yakovlev <stas.yakovlev@...il.com>,
        Kalle Valo <kvalo@...nel.org>, linux-wireless@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] wifi: ipw2x00: replace deprecated strncpy with strscpy

On Tue, Aug 1, 2023 at 4:25 PM Kees Cook <keescook@...omium.org> wrote:
>
> On Tue, Aug 01, 2023 at 09:53:36PM +0000, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > We can massively simplify the implementation by removing the ternary
> > check for the smaller of `count` and `sizeof(buffer) - 1` as `strscpy`
> > guarantees NUL-termination of its destination buffer [2]. This also
> > means we do not need to explicity set the one past-the-last index to
> > zero as `strscpy` handles this.
> >
> > Furthermore, we can also utilize `strscpy`'s return value to populate
> > `len` and simply pass in `sizeof(buffer)` to the `strscpy` invocation
> > itself.
> >
> > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > [2]: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> >
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@...r.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> > ---
> >  drivers/net/wireless/intel/ipw2x00/ipw2200.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.c b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> > index dfe0f74369e6..8f2a834dbe04 100644
> > --- a/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> > +++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> > @@ -1462,15 +1462,12 @@ static ssize_t scan_age_store(struct device *d, struct device_attribute *attr,
> >       struct ipw_priv *priv = dev_get_drvdata(d);
> >       struct net_device *dev = priv->net_dev;
> >       char buffer[] = "00000000";
> > -     unsigned long len =
> > -         (sizeof(buffer) - 1) > count ? count : sizeof(buffer) - 1;
> >       unsigned long val;
> >       char *p = buffer;
> >
> >       IPW_DEBUG_INFO("enter\n");
> >
> > -     strncpy(buffer, buf, len);
> > -     buffer[len] = 0;
> > +     ssize_t len = strscpy(buffer, buf, sizeof(buffer));
>
> This means "len" could become -E2BIG, which changes the behavior of this
> function. The earlier manipulation of "len" seems to be trying to
> explicitly allow for truncation, though. (if buffer could hold more than
> "count", copy "count", otherwise copy less)
>
> So it looks like -E2BIG should be ignored here? But since this is a
> sysfs node (static DEVICE_ATTR_RW(scan_age)), I actually think the
> original code may be bugged: it should return how much was read from
> the input... and technically this was true, but it seems the intent
> is to consume the entire buffer and set a result. It's possible "len"
> is entirely unneeded and this should just return "count"?
>
> And, honestly, I think it's likely that most of this entire routine should
> be thrown out in favor of just using kstrtoul() with base 0, as sysfs
> input buffers are always NUL-terminated. (See kernfs_fop_write_iter().)

Great suggestion, instead of v2'ing this patch I've opted to create a
new one due to it being slightly larger scope than just replacing
`strncpy`.

Patch:  https://lore.kernel.org/r/20230802-wifi-ipw2x00-refactor-v1-1-6047659410d4@google.com

>
>
> diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.c b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> index dfe0f74369e6..780f5613e279 100644
> --- a/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> +++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.c
> @@ -1461,25 +1461,11 @@ static ssize_t scan_age_store(struct device *d, struct device_attribute *attr,
>  {
>         struct ipw_priv *priv = dev_get_drvdata(d);
>         struct net_device *dev = priv->net_dev;
> -       char buffer[] = "00000000";
> -       unsigned long len =
> -           (sizeof(buffer) - 1) > count ? count : sizeof(buffer) - 1;
>         unsigned long val;
> -       char *p = buffer;
>
>         IPW_DEBUG_INFO("enter\n");
>
> -       strncpy(buffer, buf, len);
> -       buffer[len] = 0;
> -
> -       if (p[1] == 'x' || p[1] == 'X' || p[0] == 'x' || p[0] == 'X') {
> -               p++;
> -               if (p[0] == 'x' || p[0] == 'X')
> -                       p++;
> -               val = simple_strtoul(p, &p, 16);
> -       } else
> -               val = simple_strtoul(p, &p, 10);
> -       if (p == buffer) {
> +       if (kstrtoul(buf, 0, &val)) {
>                 IPW_DEBUG_INFO("%s: user supplied invalid value.\n", dev->name);
>         } else {
>                 priv->ieee->scan_age = val;
> @@ -1487,7 +1473,7 @@ static ssize_t scan_age_store(struct device *d, struct device_attribute *attr,
>         }
>
>         IPW_DEBUG_INFO("exit\n");
> -       return len;
> +       return count;
>  }
>
>  static DEVICE_ATTR_RW(scan_age);
>
>
> -Kees
>
> >
> >       if (p[1] == 'x' || p[1] == 'X' || p[0] == 'x' || p[0] == 'X') {
> >               p++;
> >
> > ---
> > base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
> > change-id: 20230801-drivers-net-wireless-intel-ipw2x00-d7ee2dd17032
> >
> > Best regards,
> > --
> > Justin Stitt <justinstitt@...gle.com>
> >
>
> --
> Kees Cook