Messages in this thread Patch in this message | | | Date | Tue, 1 Aug 2023 22:26:51 +0100 | From | Conor Dooley <> | Subject | Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy |
| |
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. [unhandled content-type:application/pgp-signature] | |