| Message ID | 20230911-strncpy-drivers-block-aoe-aoenet-c-v1-1-9643d6137ff9@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | aoe: refactor deprecated strncpy | expand |
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> >
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.
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 >
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.
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; }
`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(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-drivers-block-aoe-aoenet-c-024debad6105 Best regards, -- Justin Stitt <justinstitt@google.com>