linux-kernel - Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFhGd8pCf5Ose-AVH2k-3fP=7AkHsX5eDsd82Z0F5qEdUFT0qQ@mail.gmail.com>
Date:   Tue, 1 Aug 2023 15:22:07 -0700
From:   Justin Stitt <justinstitt@...gle.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy

On Tue, Aug 1, 2023 at 2:26 PM Conor Dooley <conor@...nel.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@...r.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@...gle.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@...rochip.com>
>
> Thanks,
> Conor.

--
Justin