On Tue, Aug 1, 2023 at 4:25 PM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx > > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx > > > 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@xxxxxxxxxx> > > > > -- > Kees Cook
