* [PATCH] arm64/sysreg: refactor deprecated strncpy @ 2023-08-10 18:39 Justin Stitt 2023-08-10 19:00 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Justin Stitt @ 2023-08-10 18:39 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: linux-arm-kernel, linux-kernel, Kees Cook, linux-hardening, Justin Stitt `strncpy` is deprecated for use on NUL-terminated destination strings [1]. Which seems to be the case here due to the forceful setting of `buf`'s tail to 0. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ the case for `strncpy`! In this case, there is some behavior being used in conjunction with `strncpy` that `strscpy` already implements. This means we can drop some of the extra stuff like `... -1` and `buf[len] = 0` This should have no functional change and yet uses a more robust and less ambiguous interface whilst reducing code complexity. Link: 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> --- For reference, see a part of `strscpy`'s implementation here: | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; Note: compile tested --- arch/arm64/kernel/idreg-override.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index 2fe2491b692c..482dc5c71e90 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) if (!len) return; - len = min(len, ARRAY_SIZE(buf) - 1); - strncpy(buf, cmdline, len); - buf[len] = 0; + len = min(len, ARRAY_SIZE(buf)); + strscpy(buf, cmdline, len); if (strcmp(buf, "--") == 0) return; --- base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f change-id: 20230810-strncpy-arch-arm64-1f3d328bd9b8 Best regards, -- Justin Stitt <justinstitt@google.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/sysreg: refactor deprecated strncpy 2023-08-10 18:39 [PATCH] arm64/sysreg: refactor deprecated strncpy Justin Stitt @ 2023-08-10 19:00 ` Kees Cook 2023-08-10 19:25 ` Justin Stitt 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2023-08-10 19:00 UTC (permalink / raw) To: Justin Stitt Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, linux-hardening On Thu, Aug 10, 2023 at 06:39:03PM +0000, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings > [1]. Which seems to be the case here due to the forceful setting of `buf`'s > tail to 0. Another note to include in these evaluations would be "does the destination expect to be %NUL padded?". Here, it looks like no, as all the routines "buf" is passed to expect a regular C string (padding doesn't matter). > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on its destination buffer argument which is > _not_ the case for `strncpy`! > > In this case, there is some behavior being used in conjunction with > `strncpy` that `strscpy` already implements. This means we can drop some > of the extra stuff like `... -1` and `buf[len] = 0` > > This should have no functional change and yet uses a more robust and > less ambiguous interface whilst reducing code complexity. > > Link: 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> > --- > For reference, see a part of `strscpy`'s implementation here: > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > | if (res) > | dest[res-1] = '\0'; > > Note: compile tested > --- > arch/arm64/kernel/idreg-override.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > index 2fe2491b692c..482dc5c71e90 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > if (!len) > return; > > - len = min(len, ARRAY_SIZE(buf) - 1); > - strncpy(buf, cmdline, len); > - buf[len] = 0; > + len = min(len, ARRAY_SIZE(buf)); > + strscpy(buf, cmdline, len); This, however, isn't correct: "cmdline" will be incremented by "leN" later, and we want a count of the characters copied into "buf", even if they're truncated. I think this should be: strscpy(buf, cmdline, ARRAY_SIZE(buf)); len = strlen(buf); -Kees > > if (strcmp(buf, "--") == 0) > return; > > --- > base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f > change-id: 20230810-strncpy-arch-arm64-1f3d328bd9b8 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/sysreg: refactor deprecated strncpy 2023-08-10 19:00 ` Kees Cook @ 2023-08-10 19:25 ` Justin Stitt 2023-08-10 19:58 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Justin Stitt @ 2023-08-10 19:25 UTC (permalink / raw) To: Kees Cook Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, linux-hardening On Thu, Aug 10, 2023 at 12:00 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Aug 10, 2023 at 06:39:03PM +0000, Justin Stitt wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings > > [1]. Which seems to be the case here due to the forceful setting of `buf`'s > > tail to 0. > > Another note to include in these evaluations would be "does the > destination expect to be %NUL padded?". Here, it looks like no, as all > the routines "buf" is passed to expect a regular C string (padding > doesn't matter). > > > > > A suitable replacement is `strscpy` [2] due to the fact that it > > guarantees NUL-termination on its destination buffer argument which is > > _not_ the case for `strncpy`! > > > > In this case, there is some behavior being used in conjunction with > > `strncpy` that `strscpy` already implements. This means we can drop some > > of the extra stuff like `... -1` and `buf[len] = 0` > > > > This should have no functional change and yet uses a more robust and > > less ambiguous interface whilst reducing code complexity. > > > > Link: 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> > > --- > > For reference, see a part of `strscpy`'s implementation here: > > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > > | if (res) > > | dest[res-1] = '\0'; > > > > Note: compile tested > > --- > > arch/arm64/kernel/idreg-override.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > > index 2fe2491b692c..482dc5c71e90 100644 > > --- a/arch/arm64/kernel/idreg-override.c > > +++ b/arch/arm64/kernel/idreg-override.c > > @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > > if (!len) > > return; > > > > - len = min(len, ARRAY_SIZE(buf) - 1); > > - strncpy(buf, cmdline, len); > > - buf[len] = 0; > > + len = min(len, ARRAY_SIZE(buf)); > > + strscpy(buf, cmdline, len); > > This, however, isn't correct: "cmdline" will be incremented by "leN" > later, and we want a count of the characters copied into "buf", even if > they're truncated. I think this should be: > > strscpy(buf, cmdline, ARRAY_SIZE(buf)); > len = strlen(buf); > Thoughts on using the return value from `strscpy` here? > -Kees > > > > > if (strcmp(buf, "--") == 0) > > return; > > > > --- > > base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f > > change-id: 20230810-strncpy-arch-arm64-1f3d328bd9b8 > > > > Best regards, > > -- > > Justin Stitt <justinstitt@google.com> > > > > -- > Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/sysreg: refactor deprecated strncpy 2023-08-10 19:25 ` Justin Stitt @ 2023-08-10 19:58 ` Kees Cook 2023-08-10 21:17 ` Justin Stitt 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2023-08-10 19:58 UTC (permalink / raw) To: Justin Stitt Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, linux-hardening On Thu, Aug 10, 2023 at 12:25:37PM -0700, Justin Stitt wrote: > On Thu, Aug 10, 2023 at 12:00 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Aug 10, 2023 at 06:39:03PM +0000, Justin Stitt wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings > > > [1]. Which seems to be the case here due to the forceful setting of `buf`'s > > > tail to 0. > > > > Another note to include in these evaluations would be "does the > > destination expect to be %NUL padded?". Here, it looks like no, as all > > the routines "buf" is passed to expect a regular C string (padding > > doesn't matter). > > > > > > > > A suitable replacement is `strscpy` [2] due to the fact that it > > > guarantees NUL-termination on its destination buffer argument which is > > > _not_ the case for `strncpy`! > > > > > > In this case, there is some behavior being used in conjunction with > > > `strncpy` that `strscpy` already implements. This means we can drop some > > > of the extra stuff like `... -1` and `buf[len] = 0` > > > > > > This should have no functional change and yet uses a more robust and > > > less ambiguous interface whilst reducing code complexity. > > > > > > Link: 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> > > > --- > > > For reference, see a part of `strscpy`'s implementation here: > > > > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > > > | if (res) > > > | dest[res-1] = '\0'; > > > > > > Note: compile tested > > > --- > > > arch/arm64/kernel/idreg-override.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > > > index 2fe2491b692c..482dc5c71e90 100644 > > > --- a/arch/arm64/kernel/idreg-override.c > > > +++ b/arch/arm64/kernel/idreg-override.c > > > @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > > > if (!len) > > > return; > > > > > > - len = min(len, ARRAY_SIZE(buf) - 1); > > > - strncpy(buf, cmdline, len); > > > - buf[len] = 0; > > > + len = min(len, ARRAY_SIZE(buf)); > > > + strscpy(buf, cmdline, len); > > > > This, however, isn't correct: "cmdline" will be incremented by "leN" > > later, and we want a count of the characters copied into "buf", even if > > they're truncated. I think this should be: > > > > strscpy(buf, cmdline, ARRAY_SIZE(buf)); > > len = strlen(buf); > > > Thoughts on using the return value from `strscpy` here? This code seems to silently accept truncation, so -E2BIG will cause a problem if it only looks at the return value. -Kees -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/sysreg: refactor deprecated strncpy 2023-08-10 19:58 ` Kees Cook @ 2023-08-10 21:17 ` Justin Stitt 2023-08-11 0:42 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Justin Stitt @ 2023-08-10 21:17 UTC (permalink / raw) To: Kees Cook Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, linux-hardening On Thu, Aug 10, 2023 at 12:58 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Aug 10, 2023 at 12:25:37PM -0700, Justin Stitt wrote: > > On Thu, Aug 10, 2023 at 12:00 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Thu, Aug 10, 2023 at 06:39:03PM +0000, Justin Stitt wrote: > > > > `strncpy` is deprecated for use on NUL-terminated destination strings > > > > [1]. Which seems to be the case here due to the forceful setting of `buf`'s > > > > tail to 0. > > > > > > Another note to include in these evaluations would be "does the > > > destination expect to be %NUL padded?". Here, it looks like no, as all > > > the routines "buf" is passed to expect a regular C string (padding > > > doesn't matter). > > > > > > > > > > > A suitable replacement is `strscpy` [2] due to the fact that it > > > > guarantees NUL-termination on its destination buffer argument which is > > > > _not_ the case for `strncpy`! > > > > > > > > In this case, there is some behavior being used in conjunction with > > > > `strncpy` that `strscpy` already implements. This means we can drop some > > > > of the extra stuff like `... -1` and `buf[len] = 0` > > > > > > > > This should have no functional change and yet uses a more robust and > > > > less ambiguous interface whilst reducing code complexity. > > > > > > > > Link: 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> > > > > --- > > > > For reference, see a part of `strscpy`'s implementation here: > > > > > > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ > > > > | if (res) > > > > | dest[res-1] = '\0'; > > > > > > > > Note: compile tested > > > > --- > > > > arch/arm64/kernel/idreg-override.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > > > > index 2fe2491b692c..482dc5c71e90 100644 > > > > --- a/arch/arm64/kernel/idreg-override.c > > > > +++ b/arch/arm64/kernel/idreg-override.c > > > > @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > > > > if (!len) > > > > return; > > > > > > > > - len = min(len, ARRAY_SIZE(buf) - 1); > > > > - strncpy(buf, cmdline, len); > > > > - buf[len] = 0; > > > > + len = min(len, ARRAY_SIZE(buf)); > > > > + strscpy(buf, cmdline, len); Perhaps keeping the `... - 1` is good because we then don't have to check strlen immediately after. This does still silently truncate but didn't the previous `strncpy` also do that? > > > > > > This, however, isn't correct: "cmdline" will be incremented by "leN" > > > later, and we want a count of the characters copied into "buf", even if > > > they're truncated. I think this should be: > > > > > > strscpy(buf, cmdline, ARRAY_SIZE(buf)); > > > len = strlen(buf); > > > > > Thoughts on using the return value from `strscpy` here? > > This code seems to silently accept truncation, so -E2BIG will cause a > problem if it only looks at the return value. > > -Kees > > -- > Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/sysreg: refactor deprecated strncpy 2023-08-10 21:17 ` Justin Stitt @ 2023-08-11 0:42 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2023-08-11 0:42 UTC (permalink / raw) To: Justin Stitt, Kees Cook Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, linux-hardening On August 10, 2023 2:17:41 PM PDT, Justin Stitt <justinstitt@google.com> wrote: >On Thu, Aug 10, 2023 at 12:58 PM Kees Cook <keescook@chromium.org> wrote: >> >> On Thu, Aug 10, 2023 at 12:25:37PM -0700, Justin Stitt wrote: >> > On Thu, Aug 10, 2023 at 12:00 PM Kees Cook <keescook@chromium.org> wrote: >> > > >> > > On Thu, Aug 10, 2023 at 06:39:03PM +0000, Justin Stitt wrote: >> > > > `strncpy` is deprecated for use on NUL-terminated destination strings >> > > > [1]. Which seems to be the case here due to the forceful setting of `buf`'s >> > > > tail to 0. >> > > >> > > Another note to include in these evaluations would be "does the >> > > destination expect to be %NUL padded?". Here, it looks like no, as all >> > > the routines "buf" is passed to expect a regular C string (padding >> > > doesn't matter). >> > > >> > > > >> > > > A suitable replacement is `strscpy` [2] due to the fact that it >> > > > guarantees NUL-termination on its destination buffer argument which is >> > > > _not_ the case for `strncpy`! >> > > > >> > > > In this case, there is some behavior being used in conjunction with >> > > > `strncpy` that `strscpy` already implements. This means we can drop some >> > > > of the extra stuff like `... -1` and `buf[len] = 0` >> > > > >> > > > This should have no functional change and yet uses a more robust and >> > > > less ambiguous interface whilst reducing code complexity. >> > > > >> > > > Link: 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> >> > > > --- >> > > > For reference, see a part of `strscpy`'s implementation here: >> > > > >> > > > | /* Hit buffer length without finding a NUL; force NUL-termination. */ >> > > > | if (res) >> > > > | dest[res-1] = '\0'; >> > > > >> > > > Note: compile tested >> > > > --- >> > > > arch/arm64/kernel/idreg-override.c | 5 ++--- >> > > > 1 file changed, 2 insertions(+), 3 deletions(-) >> > > > >> > > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c >> > > > index 2fe2491b692c..482dc5c71e90 100644 >> > > > --- a/arch/arm64/kernel/idreg-override.c >> > > > +++ b/arch/arm64/kernel/idreg-override.c >> > > > @@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) >> > > > if (!len) >> > > > return; >> > > > >> > > > - len = min(len, ARRAY_SIZE(buf) - 1); >> > > > - strncpy(buf, cmdline, len); >> > > > - buf[len] = 0; >> > > > + len = min(len, ARRAY_SIZE(buf)); >> > > > + strscpy(buf, cmdline, len); >Perhaps keeping the `... - 1` is good because we then don't have to >check strlen immediately after. This does still silently truncate but >didn't the previous `strncpy` also do that? Ah, actually there's no need to get too tricky here. This should be behaviorally identical: len = strscpy(buf, cmdline, ARRAY_SIZE(buf)); if (len == -E2BIG) len = ARRAY_SIZE(buf) - 1; -Kees -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-11 0:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-10 18:39 [PATCH] arm64/sysreg: refactor deprecated strncpy Justin Stitt 2023-08-10 19:00 ` Kees Cook 2023-08-10 19:25 ` Justin Stitt 2023-08-10 19:58 ` Kees Cook 2023-08-10 21:17 ` Justin Stitt 2023-08-11 0:42 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox