* Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
2023-08-01 21:14 [PATCH] RISC-V: cpu: refactor deprecated strncpy Justin Stitt
@ 2023-08-01 21:26 ` Conor Dooley
2023-08-01 22:22 ` Justin Stitt
2023-08-01 23:02 ` Jessica Clarke
2023-08-01 23:33 ` Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Conor Dooley @ 2023-08-01 21:26 UTC (permalink / raw)
To: Justin Stitt
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, Kees Cook, Nick Desaulniers, linux-hardening
[-- Attachment #1: Type: text/plain, Size: 3621 bytes --]
Hey Justin,
On Tue, Aug 01, 2023 at 09:14:56PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> 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`!
>
> The `sv_type` buffer is declared with a size of 16 which is then
> followed by some `strncpy` calls to populate the buffer with one of:
> "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> error-prone and involves counting the number of characters (and
> hopefully not forgetting to count the NUL-byte) in the raw string.
What is error prone about it when there are only 4 characters possible?
> Using a pre-determined max length in combination with `strscpy` provides
> a cleaner, less error-prone as well as a less ambiguous implementation.
> `strscpy` guarantees that it's destination buffer is NUL-terminated even
> if it's source argument exceeds the max length as defined by the third
Wrong its ;)
> argument.
>
> To be clear, there is no bug (i think?) in the current implementation
> but the current hard-coded values in combination with using a deprecated
> interface make this a worthwhile change, IMO.
>
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
This link is broken, it should be
https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
Also, in the future, please use the form
Link: <url> [ref]
so
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [1]
and so on.
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> arch/riscv/kernel/cpu.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..1c576e4ec171 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -17,6 +17,8 @@
> #include <asm/smp.h>
> #include <asm/pgtable.h>
>
> +#define SV_TYPE_MAX_LENGTH 16
> +
> /*
> * Returns the hart ID of the given device tree node, or -ENODEV if the node
> * isn't an enabled and valid RISC-V hart node.
> @@ -271,21 +273,21 @@ static void print_isa(struct seq_file *f, const char *isa)
>
> static void print_mmu(struct seq_file *f)
> {
> - char sv_type[16];
> + char sv_type[SV_TYPE_MAX_LENGTH];
>
> #ifdef CONFIG_MMU
> #if defined(CONFIG_32BIT)
> - strncpy(sv_type, "sv32", 5);
> + strscpy(sv_type, "sv32", SV_TYPE_MAX_LENGTH);
> #elif defined(CONFIG_64BIT)
> if (pgtable_l5_enabled)
> - strncpy(sv_type, "sv57", 5);
> + strscpy(sv_type, "sv57", SV_TYPE_MAX_LENGTH);
> else if (pgtable_l4_enabled)
> - strncpy(sv_type, "sv48", 5);
> + strscpy(sv_type, "sv48", SV_TYPE_MAX_LENGTH);
> else
> - strncpy(sv_type, "sv39", 5);
> + strscpy(sv_type, "sv39", SV_TYPE_MAX_LENGTH);
> #endif
> #else
> - strncpy(sv_type, "none", 5);
> + strscpy(sv_type, "none", SV_TYPE_MAX_LENGTH);
> #endif /* CONFIG_MMU */
> seq_printf(f, "mmu\t\t: %s\n", sv_type);
> }
This all seems rather horrible, we should probably clean it up, but that
is nothing to do with your patch. To be clear, I am also not requesting a
resubmission for the commit message nitpickery.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
2023-08-01 21:26 ` Conor Dooley
@ 2023-08-01 22:22 ` Justin Stitt
0 siblings, 0 replies; 6+ messages in thread
From: Justin Stitt @ 2023-08-01 22:22 UTC (permalink / raw)
To: Conor Dooley
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, Kees Cook, Nick Desaulniers, linux-hardening
On Tue, Aug 1, 2023 at 2:26 PM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Justin,
>
> On Tue, Aug 01, 2023 at 09:14:56PM +0000, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > 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`!
> >
> > The `sv_type` buffer is declared with a size of 16 which is then
> > followed by some `strncpy` calls to populate the buffer with one of:
> > "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> > error-prone and involves counting the number of characters (and
> > hopefully not forgetting to count the NUL-byte) in the raw string.
>
> What is error prone about it when there are only 4 characters possible?
To clarify, I don't believe there is a bug in the current
implementation. However, what I believe to be
error prone is that simply miscounting the length of the raw string
would result in a buffer overread.
See here: https://godbolt.org/z/5reW7a1sz
>
> > Using a pre-determined max length in combination with `strscpy` provides
> > a cleaner, less error-prone as well as a less ambiguous implementation.
> > `strscpy` guarantees that it's destination buffer is NUL-terminated even
> > if it's source argument exceeds the max length as defined by the third
>
> Wrong its ;)
Oops, English is hard ;(
>
> > argument.
> >
> > To be clear, there is no bug (i think?) in the current implementation
> > but the current hard-coded values in combination with using a deprecated
> > interface make this a worthwhile change, IMO.
> >
> > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
>
> This link is broken, it should be
> https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
Noted, will fix in future patches.
>
> Also, in the future, please use the form
>
> Link: <url> [ref]
>
> so
>
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [1]
>
> and so on.
Got it. Thanks!
>
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> > arch/riscv/kernel/cpu.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index a2fc952318e9..1c576e4ec171 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -17,6 +17,8 @@
> > #include <asm/smp.h>
> > #include <asm/pgtable.h>
> >
> > +#define SV_TYPE_MAX_LENGTH 16
> > +
> > /*
> > * Returns the hart ID of the given device tree node, or -ENODEV if the node
> > * isn't an enabled and valid RISC-V hart node.
> > @@ -271,21 +273,21 @@ static void print_isa(struct seq_file *f, const char *isa)
> >
> > static void print_mmu(struct seq_file *f)
> > {
> > - char sv_type[16];
> > + char sv_type[SV_TYPE_MAX_LENGTH];
> >
> > #ifdef CONFIG_MMU
> > #if defined(CONFIG_32BIT)
> > - strncpy(sv_type, "sv32", 5);
> > + strscpy(sv_type, "sv32", SV_TYPE_MAX_LENGTH);
> > #elif defined(CONFIG_64BIT)
> > if (pgtable_l5_enabled)
> > - strncpy(sv_type, "sv57", 5);
> > + strscpy(sv_type, "sv57", SV_TYPE_MAX_LENGTH);
> > else if (pgtable_l4_enabled)
> > - strncpy(sv_type, "sv48", 5);
> > + strscpy(sv_type, "sv48", SV_TYPE_MAX_LENGTH);
> > else
> > - strncpy(sv_type, "sv39", 5);
> > + strscpy(sv_type, "sv39", SV_TYPE_MAX_LENGTH);
> > #endif
> > #else
> > - strncpy(sv_type, "none", 5);
> > + strscpy(sv_type, "none", SV_TYPE_MAX_LENGTH);
> > #endif /* CONFIG_MMU */
> > seq_printf(f, "mmu\t\t: %s\n", sv_type);
> > }
>
> This all seems rather horrible, we should probably clean it up, but that
> is nothing to do with your patch. To be clear, I am also not requesting a
> resubmission for the commit message nitpickery.
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> Thanks,
> Conor.
--
Justin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
2023-08-01 21:14 [PATCH] RISC-V: cpu: refactor deprecated strncpy Justin Stitt
2023-08-01 21:26 ` Conor Dooley
@ 2023-08-01 23:02 ` Jessica Clarke
2023-08-01 23:30 ` Kees Cook
2023-08-01 23:33 ` Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Jessica Clarke @ 2023-08-01 23:02 UTC (permalink / raw)
To: Justin Stitt
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv, LKML,
Kees Cook, Nick Desaulniers, linux-hardening
On 1 Aug 2023, at 22:14, Justin Stitt <justinstitt@google.com> wrote:
>
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> 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`!
>
> The `sv_type` buffer is declared with a size of 16 which is then
> followed by some `strncpy` calls to populate the buffer with one of:
> "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> error-prone and involves counting the number of characters (and
> hopefully not forgetting to count the NUL-byte) in the raw string.
>
> Using a pre-determined max length in combination with `strscpy` provides
> a cleaner, less error-prone as well as a less ambiguous implementation.
> `strscpy` guarantees that it's destination buffer is NUL-terminated even
> if it's source argument exceeds the max length as defined by the third
> argument.
I would imagine you’d want a BUG_ON() rather than silent truncation if
that ever happened (well, silent if you ignore it then printing the
truncated string).
Though really you just want a static_strcpy that looks at sizeof* for
source and destination and fails to build if it doesn’t fit; there’s no
reason this needs to be found at run time.
(* and __builtin_types_compatible_p(char[], ...))
Jess
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
2023-08-01 23:02 ` Jessica Clarke
@ 2023-08-01 23:30 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-08-01 23:30 UTC (permalink / raw)
To: Jessica Clarke
Cc: Justin Stitt, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, LKML, Nick Desaulniers, linux-hardening
On Wed, Aug 02, 2023 at 12:02:11AM +0100, Jessica Clarke wrote:
> On 1 Aug 2023, at 22:14, Justin Stitt <justinstitt@google.com> wrote:
> >
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > 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`!
> >
> > The `sv_type` buffer is declared with a size of 16 which is then
> > followed by some `strncpy` calls to populate the buffer with one of:
> > "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> > error-prone and involves counting the number of characters (and
> > hopefully not forgetting to count the NUL-byte) in the raw string.
> >
> > Using a pre-determined max length in combination with `strscpy` provides
> > a cleaner, less error-prone as well as a less ambiguous implementation.
> > `strscpy` guarantees that it's destination buffer is NUL-terminated even
> > if it's source argument exceeds the max length as defined by the third
> > argument.
>
> I would imagine you’d want a BUG_ON() rather than silent truncation if
> that ever happened (well, silent if you ignore it then printing the
> truncated string).
>
> Though really you just want a static_strcpy that looks at sizeof* for
> source and destination and fails to build if it doesn’t fit; there’s no
> reason this needs to be found at run time.
FWIW, under CONFIG_FORTIFY_SOURCE, strscpy() does try to just fold away
to a static strcpy when sizes are provably safe, etc.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
2023-08-01 21:14 [PATCH] RISC-V: cpu: refactor deprecated strncpy Justin Stitt
2023-08-01 21:26 ` Conor Dooley
2023-08-01 23:02 ` Jessica Clarke
@ 2023-08-01 23:33 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-08-01 23:33 UTC (permalink / raw)
To: Justin Stitt
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, Nick Desaulniers, linux-hardening
On Tue, Aug 01, 2023 at 09:14:56PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> 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`!
>
> The `sv_type` buffer is declared with a size of 16 which is then
> followed by some `strncpy` calls to populate the buffer with one of:
> "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> error-prone and involves counting the number of characters (and
> hopefully not forgetting to count the NUL-byte) in the raw string.
>
> Using a pre-determined max length in combination with `strscpy` provides
> a cleaner, less error-prone as well as a less ambiguous implementation.
> `strscpy` guarantees that it's destination buffer is NUL-terminated even
> if it's source argument exceeds the max length as defined by the third
> argument.
>
> To be clear, there is no bug (i think?) in the current implementation
> but the current hard-coded values in combination with using a deprecated
> interface make this a worthwhile change, IMO.
>
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
>
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> arch/riscv/kernel/cpu.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..1c576e4ec171 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -17,6 +17,8 @@
> #include <asm/smp.h>
> #include <asm/pgtable.h>
>
> +#define SV_TYPE_MAX_LENGTH 16
> +
> /*
> * Returns the hart ID of the given device tree node, or -ENODEV if the node
> * isn't an enabled and valid RISC-V hart node.
> @@ -271,21 +273,21 @@ static void print_isa(struct seq_file *f, const char *isa)
>
> static void print_mmu(struct seq_file *f)
> {
> - char sv_type[16];
> + char sv_type[SV_TYPE_MAX_LENGTH];
>
> #ifdef CONFIG_MMU
> #if defined(CONFIG_32BIT)
> - strncpy(sv_type, "sv32", 5);
> + strscpy(sv_type, "sv32", SV_TYPE_MAX_LENGTH);
> #elif defined(CONFIG_64BIT)
> if (pgtable_l5_enabled)
> - strncpy(sv_type, "sv57", 5);
> + strscpy(sv_type, "sv57", SV_TYPE_MAX_LENGTH);
> else if (pgtable_l4_enabled)
> - strncpy(sv_type, "sv48", 5);
> + strscpy(sv_type, "sv48", SV_TYPE_MAX_LENGTH);
> else
> - strncpy(sv_type, "sv39", 5);
> + strscpy(sv_type, "sv39", SV_TYPE_MAX_LENGTH);
> #endif
> #else
> - strncpy(sv_type, "none", 5);
> + strscpy(sv_type, "none", SV_TYPE_MAX_LENGTH);
> #endif /* CONFIG_MMU */
> seq_printf(f, "mmu\t\t: %s\n", sv_type);
> }
I'd say just throw the whole buffer away and just avoid copying the
.rodata strings onto the stack for no reason. They can be used directly:
static void print_mmu(struct seq_file *f)
{
const char *sv_type;
#ifdef CONFIG_MMU
#if defined(CONFIG_32BIT)
sv_type = "sv32";
#elif defined(CONFIG_64BIT)
if (pgtable_l5_enabled)
sv_type = "sv57";
else if (pgtable_l4_enabled)
sv_type = "sv48";
else
sv_type = "sv39";
#endif
#else
sv_type = "none";
#endif /* CONFIG_MMU */
seq_printf(f, "mmu\t\t: %s\n", sv_type);
}
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread