| Message ID | 20230914-strncpy-drivers-hwmon-ibmpowernv-c-v1-1-ba6b7f42c98c@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (ibmpowernv) refactor deprecated strncpy | expand |
On Thu, Sep 14, 2023 at 11:21:06PM +0000, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding since `buf` is already zero-initialized. > > 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> > --- > drivers/hwmon/ibmpowernv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c > index 594254d6a72d..57d829dbcda6 100644 > --- a/drivers/hwmon/ibmpowernv.c > +++ b/drivers/hwmon/ibmpowernv.c > @@ -234,7 +234,7 @@ static int get_sensor_index_attr(const char *name, u32 *index, char *attr) > if (copy_len >= sizeof(buf)) > return -EINVAL; > > - strncpy(buf, hash_pos + 1, copy_len); > + strscpy(buf, hash_pos + 1, copy_len); This is another case of precise byte copying -- this just needs to be memcpy. Otherwise this truncates the trailing character. Imagine a name input of "fan#2-data". "buf" wants to get "2". copy_len is 1, and strscpy would eat it. :) -Kees > > err = kstrtou32(buf, 10, index); > if (err) > > --- > base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec > change-id: 20230914-strncpy-drivers-hwmon-ibmpowernv-c-80a03f16d93a > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
On 9/14/23 22:24, Kees Cook wrote: > On Thu, Sep 14, 2023 at 11:21:06PM +0000, Justin Stitt wrote: >> `strncpy` is deprecated for use on NUL-terminated destination strings [1]. >> >> We should prefer more robust and less ambiguous string interfaces. >> >> A suitable replacement is `strscpy` [2] due to the fact that it >> guarantees NUL-termination on the destination buffer without >> unnecessarily NUL-padding since `buf` is already zero-initialized. >> >> 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> >> --- >> drivers/hwmon/ibmpowernv.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >> index 594254d6a72d..57d829dbcda6 100644 >> --- a/drivers/hwmon/ibmpowernv.c >> +++ b/drivers/hwmon/ibmpowernv.c >> @@ -234,7 +234,7 @@ static int get_sensor_index_attr(const char *name, u32 *index, char *attr) >> if (copy_len >= sizeof(buf)) >> return -EINVAL; >> >> - strncpy(buf, hash_pos + 1, copy_len); >> + strscpy(buf, hash_pos + 1, copy_len); > > This is another case of precise byte copying -- this just needs to be > memcpy. Otherwise this truncates the trailing character. Imagine a name > input of "fan#2-data". "buf" wants to get "2". copy_len is 1, and > strscpy would eat it. :) > It is really sad that the submitters of such "cleanup" patches can't be bothered to check what they are doing. They can't even be bothered to write a coccinelle script that would avoid pitfalls like this one, and they expect others to do their homework for them. And then people wonder why there is maintainer burnout. I am so tired of that. Guenter
On Thu, Sep 14, 2023 at 10:40:37PM -0700, Guenter Roeck wrote: > It is really sad that the submitters of such "cleanup" patches can't be bothered > to check what they are doing. They can't even be bothered to write a coccinelle > script that would avoid pitfalls like this one, and they expect others to do their > homework for them. Well I'm not sure that's entirely fair to Justin's efforts (I know he's been studying these changes and everyone makes mistakes), but that's why I'm helping review his findings -- some code behaviors are more obvious than others. :)
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c index 594254d6a72d..57d829dbcda6 100644 --- a/drivers/hwmon/ibmpowernv.c +++ b/drivers/hwmon/ibmpowernv.c @@ -234,7 +234,7 @@ static int get_sensor_index_attr(const char *name, u32 *index, char *attr) if (copy_len >= sizeof(buf)) return -EINVAL; - strncpy(buf, hash_pos + 1, copy_len); + strscpy(buf, hash_pos + 1, copy_len); err = kstrtou32(buf, 10, index); if (err)
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding since `buf` is already zero-initialized. 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> --- drivers/hwmon/ibmpowernv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230914-strncpy-drivers-hwmon-ibmpowernv-c-80a03f16d93a Best regards, -- Justin Stitt <justinstitt@google.com>