diff mbox series

wifi: ipw2x00: replace deprecated strncpy with strscpy

Message ID 20230801-drivers-net-wireless-intel-ipw2x00-v1-1-ffd185c91292@google.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ipw2x00: replace deprecated strncpy with strscpy | expand

Commit Message

Justin Stitt Aug. 1, 2023, 9:53 p.m. UTC 
`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>

Comments

Kees Cook Aug. 1, 2023, 11:25 p.m. UTC | #1 
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>
>
Justin Stitt Aug. 2, 2023, 12:54 a.m. UTC | #2 
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 mbox series

Patch

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++;