`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]: http://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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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);
}
---
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
change-id: 20230801-arch-riscv-kernel-14a048cc6467
Best regards,
--
Justin Stitt <[email protected]>
On 1 Aug 2023, at 22:14, Justin Stitt <[email protected]> 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
On Wed, Aug 02, 2023 at 12:02:11AM +0100, Jessica Clarke wrote:
> On 1 Aug 2023, at 22:14, Justin Stitt <[email protected]> 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
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]: http://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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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