linux-kernel - Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing

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: <20230427-unveiling-kiwi-631e966f77cc@wendy>
Date:   Thu, 27 Apr 2023 10:04:34 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Andrew Jones <ajones@...tanamicro.com>
CC:     Conor Dooley <conor@...nel.org>, Yangyu Chen <cyy@...self.name>,
        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>,
        Wende Tan <twd2.me@...il.com>, Soha Jin <soha@...u.info>,
        Hongren Zheng <i@...ithal.me>
Subject: Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing

On Thu, Apr 27, 2023 at 09:53:19AM +0200, Andrew Jones wrote:
> On Wed, Apr 26, 2023 at 07:54:39PM +0100, Conor Dooley wrote:
> > (+CC Drew)
> > 
> > Hey Yangyu,
> > 
> > One meta-level comment - can you submit this patch + my dt-bindings
> > patch as a v2?
> > Some comments below.
> > 
> > On Tue, Apr 25, 2023 at 08:00:15PM +0800, Yangyu Chen wrote:
> > > According to RISC-V ISA specification, the ISA naming strings are case
> > > insensitive. The kernel docs require the riscv,isa string must be all
> > > lowercase to simplify parsing currently. However, this limitation is not
> > > consistent with RISC-V ISA Spec.
> > 
> > Please remove the above and cite ACPI's case-insensitivity as the
> > rationale for this change.
> > 
> > > This patch modifies the ISA string parser in the kernel to support
> > > case-insensitive ISA string parsing. It replaces `strncmp` with
> > > `strncasecmp`, replaces `islower` with `isalpha`, and wraps the
> > > dereferenced char in the parser with `tolower`.
> > > 
> > > Signed-off-by: Yangyu Chen <cyy@...self.name>
> > > ---
> > >  arch/riscv/kernel/cpu.c        |  6 ++++--
> > >  arch/riscv/kernel/cpufeature.c | 20 ++++++++++----------
> > >  2 files changed, 14 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index 8400f0cc9704..531c76079b73 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -4,6 +4,7 @@
> > >   */
> > >  
> > >  #include <linux/cpu.h>
> > > +#include <linux/ctype.h>
> > >  #include <linux/init.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/of.h>
> > > @@ -41,7 +42,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
> > >  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> > >  		return -ENODEV;
> > >  	}
> > > -	if (isa[0] != 'r' || isa[1] != 'v') {
> > > +	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> > >  		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> > >  		return -ENODEV;
> > 
> > I don't understand why this is even here in the first place. I'd be
> > inclined to advocate for it's entire removal. Checking *only* that there
> > is an "rv" in that string seems pointless to me. If you're on a 64-bit
> > kernel and the node has riscv,isa = "rv32ima" it's gonna say it is okay?
> > Drew what do you think?
> 
> It makes some sense to me as a garbage detector. It's unlikely the first
> two bytes will be "rv" if the string is random junk.

Preventing the input of absolute rubbish is dt-validate's job & if the dtb
itself has been corrupted somehow I suspect that we have bigger problems
than checking for "rv" will solve.

> also do a strlen(isa) >= 4 check first, though. of_property_read_string()
> will succeed even when the string is "".

I don't think that checking that there are at least 4 characters isn't
even sufficient. Either we should confirm that this is a valid riscv,isa
to run on (so rv##ima w/ ## matching the kernel) or not bother at all.

It's a different issue though, and I'd be inclined to revisit it in the
future when the ACPI stuff is in, along with perhaps the cleanup parts
of Heiko's series too.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)