`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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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);
err = kstrtou32(buf, 10, index);
if (err)
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230914-strncpy-drivers-hwmon-ibmpowernv-c-80a03f16d93a
Best regards,
--
Justin Stitt <[email protected]>
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. :)
--
Kees Cook
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
>
--
Kees Cook
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: [email protected]
>> Signed-off-by: Justin Stitt <[email protected]>
>> ---
>> 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