Messages in this thread | | | From | Justin Stitt <> | Date | Tue, 1 Aug 2023 15:22:07 -0700 | Subject | Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy |
| |
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
| |