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@xxxxxxxxxxxxxxx
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> ---
> 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
