`strncpy` is deprecated for use on NUL-terminated destination strings [1].
`aoe_iflist` is expected to be NUL-terminated which is evident by its
use with string apis later on like `strspn`:
| p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
It also seems `aoe_iflist` does not need to be NUL-padded which means
`strscpy` [2] is a suitable replacement due to the fact that it
guarantees NUL-termination on the destination buffer while not
unnecessarily NUL-padding.
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]
Cc: Kees Cook <[email protected]>
Cc: Xu Panda <[email protected]>
Cc: Yang Yang <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
Note: This exact same patch exists [3] but seemed to die so I'm
resending. If it was actually picked-up somewhere then we can ignore
this patch.
[3]: https://lore.kernel.org/all/[email protected]/
---
drivers/block/aoe/aoenet.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
index 63773a90581d..c51ea95bc2ce 100644
--- a/drivers/block/aoe/aoenet.c
+++ b/drivers/block/aoe/aoenet.c
@@ -39,8 +39,7 @@ static struct ktstate kts;
#ifndef MODULE
static int __init aoe_iflist_setup(char *str)
{
- strncpy(aoe_iflist, str, IFLISTSZ);
- aoe_iflist[IFLISTSZ - 1] = '\0';
+ strscpy(aoe_iflist, str, IFLISTSZ);
return 1;
}
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-drivers-block-aoe-aoenet-c-024debad6105
Best regards,
--
Justin Stitt <[email protected]>
On Mon, Sep 11, 2023 at 09:09:07PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> `aoe_iflist` is expected to be NUL-terminated which is evident by its
> use with string apis later on like `strspn`:
> | p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
>
> It also seems `aoe_iflist` does not need to be NUL-padded which means
> `strscpy` [2] is a suitable replacement due to the fact that it
> guarantees NUL-termination on the destination buffer while not
> unnecessarily NUL-padding.
>
> 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]
> Cc: Kees Cook <[email protected]>
> Cc: Xu Panda <[email protected]>
> Cc: Yang Yang <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Agreed, truncation is the current behavior, and padding isn't needed.
(Or more precisely, it's already zeroed and this function is called
once.)
Reviewed-by: Kees Cook <[email protected]>
> ---
> Note: This exact same patch exists [3] but seemed to die so I'm
> resending. If it was actually picked-up somewhere then we can ignore
> this patch.
>
> [3]: https://lore.kernel.org/all/[email protected]/
Ah, weird. Well, I think this current one has a more complete commit
log, so let's use this one.
-Kees
> ---
> drivers/block/aoe/aoenet.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
> index 63773a90581d..c51ea95bc2ce 100644
> --- a/drivers/block/aoe/aoenet.c
> +++ b/drivers/block/aoe/aoenet.c
> @@ -39,8 +39,7 @@ static struct ktstate kts;
> #ifndef MODULE
> static int __init aoe_iflist_setup(char *str)
> {
> - strncpy(aoe_iflist, str, IFLISTSZ);
> - aoe_iflist[IFLISTSZ - 1] = '\0';
> + strscpy(aoe_iflist, str, IFLISTSZ);
> return 1;
> }
>
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230911-strncpy-drivers-block-aoe-aoenet-c-024debad6105
>
> Best regards,
> --
> Justin Stitt <[email protected]>
>
--
Kees Cook
On 9/14/23 9:21 PM, Kees Cook wrote:
> On Mon, Sep 11, 2023 at 09:09:07PM +0000, Justin Stitt wrote:
>> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>>
>> `aoe_iflist` is expected to be NUL-terminated which is evident by its
>> use with string apis later on like `strspn`:
>> | p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
>>
>> It also seems `aoe_iflist` does not need to be NUL-padded which means
>> `strscpy` [2] is a suitable replacement due to the fact that it
>> guarantees NUL-termination on the destination buffer while not
>> unnecessarily NUL-padding.
>>
>> 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]
>> Cc: Kees Cook <[email protected]>
>> Cc: Xu Panda <[email protected]>
>> Cc: Yang Yang <[email protected]>
>> Signed-off-by: Justin Stitt <[email protected]>
>
> Agreed, truncation is the current behavior, and padding isn't needed.
> (Or more precisely, it's already zeroed and this function is called
> once.)
>
> Reviewed-by: Kees Cook <[email protected]>
Change looks fine to me too, but for the love of $deity, please use
a proper subject line for these kinds of patches. It's not refactoring
anything.
--
Jens Axboe
On Fri, Sep 15, 2023 at 6:36 AM Jens Axboe <[email protected]> wrote:
>
> On 9/14/23 9:21 PM, Kees Cook wrote:
> > On Mon, Sep 11, 2023 at 09:09:07PM +0000, Justin Stitt wrote:
> >> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >>
> >> `aoe_iflist` is expected to be NUL-terminated which is evident by its
> >> use with string apis later on like `strspn`:
> >> | p = aoe_iflist + strspn(aoe_iflist, WHITESPACE);
> >>
> >> It also seems `aoe_iflist` does not need to be NUL-padded which means
> >> `strscpy` [2] is a suitable replacement due to the fact that it
> >> guarantees NUL-termination on the destination buffer while not
> >> unnecessarily NUL-padding.
> >>
> >> 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]
> >> Cc: Kees Cook <[email protected]>
> >> Cc: Xu Panda <[email protected]>
> >> Cc: Yang Yang <[email protected]>
> >> Signed-off-by: Justin Stitt <[email protected]>
> >
> > Agreed, truncation is the current behavior, and padding isn't needed.
> > (Or more precisely, it's already zeroed and this function is called
> > once.)
> >
> > Reviewed-by: Kees Cook <[email protected]>
>
> Change looks fine to me too, but for the love of $deity, please use
> a proper subject line for these kinds of patches. It's not refactoring
> anything.
>
Fair.
Perhaps "xyz: replace strncpy with strscpy"?
> --
> Jens Axboe
>
On 9/18/23 1:03 AM, Justin Stitt wrote:
>> Change looks fine to me too, but for the love of $deity, please use
>> a proper subject line for these kinds of patches. It's not refactoring
>> anything.
>>
>
> Fair.
>
> Perhaps "xyz: replace strncpy with strscpy"?
That's a lot more descriptive, as a) it's actually accurate, and b) this
is what the patch does. You just sent another one with this refactor
wording which makes zero sense, please resend this and others targeted
at block with a proper description.
--
Jens Axboe