[PATCH] aoe: refactor deprecated strncpy

[PATCH] aoe: refactor deprecated strncpy

[Justin Stitt]

`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: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Cc: Xu Panda <xu.panda@zte.com.cn>
Cc: Yang Yang <yang.yang29@zte.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
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/202212051930256039214@zte.com.cn/
---
 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 <justinstitt@google.com>

Re: [PATCH] aoe: refactor deprecated strncpy

[Kees Cook]

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: linux-hardening@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Xu Panda <xu.panda@zte.com.cn>
> Cc: Yang Yang <yang.yang29@zte.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

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 <keescook@chromium.org>

> ---
> 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/202212051930256039214@zte.com.cn/

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 <justinstitt@google.com>
> 

-- 
Kees Cook

Re: [PATCH] aoe: refactor deprecated strncpy

[Jens Axboe]

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: linux-hardening@vger.kernel.org
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Xu Panda <xu.panda@zte.com.cn>
>> Cc: Yang Yang <yang.yang29@zte.com>
>> Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> 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 <keescook@chromium.org>

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

Re: [PATCH] aoe: refactor deprecated strncpy

[Justin Stitt]

On Fri, Sep 15, 2023 at 6:36 AM Jens Axboe <axboe@kernel.dk> 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: linux-hardening@vger.kernel.org
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Cc: Xu Panda <xu.panda@zte.com.cn>
> >> Cc: Yang Yang <yang.yang29@zte.com>
> >> Signed-off-by: Justin Stitt <justinstitt@google.com>
> >
> > 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 <keescook@chromium.org>
>
> 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
>

Re: [PATCH] aoe: refactor deprecated strncpy

[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