| Message ID | 20230801-drivers-net-wireless-intel-ipw2x00-v1-1-ffd185c91292@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: ipw2x00: replace deprecated strncpy with strscpy | expand |
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@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.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().) 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@google.com> >
On Tue, Aug 1, 2023 at 4:25 PM Kees Cook <keescook@chromium.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@vger.kernel.org > > Signed-off-by: Justin Stitt <justinstitt@google.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@google.com> > > > > -- > Kees Cook
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)); if (p[1] == 'x' || p[1] == 'X' || p[0] == 'x' || p[0] == 'X') { p++;
`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@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- drivers/net/wireless/intel/ipw2x00/ipw2200.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) --- base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 change-id: 20230801-drivers-net-wireless-intel-ipw2x00-d7ee2dd17032 Best regards, -- Justin Stitt <justinstitt@google.com>