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]

Date:   Thu, 27 Apr 2023 11:00:16 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Yangyu Chen <cyy@...self.name>
CC:     <ajones@...tanamicro.com>, <aou@...s.berkeley.edu>,
        <conor@...nel.org>, <i@...ithal.me>,
        <linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <palmer@...belt.com>, <paul.walmsley@...ive.com>, <soha@...u.info>,
        <twd2.me@...il.com>
Subject: Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing

On Thu, Apr 27, 2023 at 05:36:25PM +0800, Yangyu Chen wrote:
> Hi, Conor

> 
> On Thu, 27 Apr 2023 10:04:34 +0100, Conor Dooley wrote:
> > 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.
> 
> What will happen if we have a bootloader in the future which allows
> overriding isa string in the DT or ACPI table, the memory corruption could
> happen if we didn't check it first.

You can do this right now, no?
You can also overwrite the memory nodes and all sorts of other things
that'll cause your system to crash too. The isa string is nothing
special in that regard ;)

> Although the kernel will not boot in this case, anything about the user
> input string should be parse carefuly that you never know what the future
> code will be but leave a checker here will remind someone who will change
> the parse in the future to check the length carefully.

of_property_read_string() will always return something that is null
terminated on success, so we can just call strncmp() to make sure that
the hart supports something usable, no?

> I have a different opinion about whether the isa string length should be
> checked.

> So I agree with drew, we should do check strlen before check the first
> two characters.

In case it was lost in translation, I was never disputing checking that
there is a string before accessing it like this, but rather questioning
why we do such a limited check here at all.

Cheers,
Conor.

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