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

[Kees Cook <keescook@chromium.org>]

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

-- 
Kees Cook