| Message ID | 20230921-strncpy-drivers-input-misc-axp20x-pek-c-v1-1-f7f6f4a5cf81@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Input: axp20x-pek - refactor deprecated strncpy | expand |
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
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);
`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(-) --- base-commit: 2cf0f715623872823a72e451243bbf555d10d032 change-id: 20230921-strncpy-drivers-input-misc-axp20x-pek-c-3c4708924bbd Best regards, -- Justin Stitt <justinstitt@google.com>