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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202308011602.3CC1C0244C@keescook>
Date:   Tue, 1 Aug 2023 16:25:01 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
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 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().)


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