Re: [PATCH] Input: axp20x-pek - refactor deprecated strncpy

From: Kees Cook <keescook@chromium.org>
To: Justin Stitt <justinstitt@google.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Chen-Yu Tsai <wens@csie.org>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] Input: axp20x-pek - refactor deprecated strncpy
Date: Sat, 23 Sep 2023 20:29:18 -0700	[thread overview]
Message-ID: <202309232024.9A31A291@keescook> (raw)
In-Reply-To: <20230921-strncpy-drivers-input-misc-axp20x-pek-c-v1-1-f7f6f4a5cf81@google.com>

On Thu, Sep 21, 2023 at 09:17:25AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> Ensuring we have a trailing NUL-byte and checking the length of bytes
> copied are both intrinsic behavior of strscpy.
> 
> Therefore, a suitable replacement is `strscpy` [2] due to the fact that
> it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> It should be noted that the original code can silently truncate and so
> can this refactoring. However, a check could be added if truncation
> is an issue:
> | len = strscpy(val_str, buf, sizeof(val_str));
> | if (len < 0) { // add this
> |   return -E2BIG; // or -EINVAL
> | }
> 
> Also, now check for `len > 0` instead of just a truthy `len` because
> `len` is now a signed type and we could run into problems if strscpy
> returned -E2BIG which would pass the truthy test.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: build-tested only.
> ---
>  drivers/input/misc/axp20x-pek.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> index 4581606a28d6..abcf78785b45 100644
> --- a/drivers/input/misc/axp20x-pek.c
> +++ b/drivers/input/misc/axp20x-pek.c
> @@ -134,16 +134,14 @@ static ssize_t axp20x_store_attr(struct device *dev,
>  {
>  	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
>  	char val_str[20];
> -	size_t len;
> +	ssize_t len;
>  	int ret, i;
>  	unsigned int val, idx = 0;
>  	unsigned int best_err = UINT_MAX;
>  
> -	val_str[sizeof(val_str) - 1] = '\0';
> -	strncpy(val_str, buf, sizeof(val_str) - 1);
> -	len = strlen(val_str);
> +	len = strscpy(val_str, buf, sizeof(val_str));
>  
> -	if (len && val_str[len - 1] == '\n')
> +	if (len > 0 && val_str[len - 1] == '\n')
>  		val_str[len - 1] = '\0';
>  
>  	ret = kstrtouint(val_str, 10, &val);

This code is doing a LOT of work before handing it off to kstrtouint(),
and none of it is needed. val_str is never used again, and the work is
to make sure the newline is dropped -- but kstrtouint() does this
already. I think this can just be:

diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 4581606a28d6..b1389a4c7702 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -134,19 +134,11 @@ static ssize_t axp20x_store_attr(struct device *dev,
 {
 	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
 	char val_str[20];
-	size_t len;
 	int ret, i;
 	unsigned int val, idx = 0;
 	unsigned int best_err = UINT_MAX;
 
-	val_str[sizeof(val_str) - 1] = '\0';
-	strncpy(val_str, buf, sizeof(val_str) - 1);
-	len = strlen(val_str);
-
-	if (len && val_str[len - 1] == '\n')
-		val_str[len - 1] = '\0';
-
-	ret = kstrtouint(val_str, 10, &val);
+	ret = kstrtouint(buf, 10, &val);
 	if (ret)
 		return ret;
 
And, [broken record] for v2, please update the Subject to better describe
the resulting change. :)

-Kees

-- 
Kees Cook
