Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Apr 2023 11:00:16 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Yangyu Chen <cyy@cyyself.name>
CC:     <ajones@ventanamicro.com>, <aou@eecs.berkeley.edu>,
        <conor@kernel.org>, <i@zenithal.me>,
        <linux-kernel@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
        <palmer@dabbelt.com>, <paul.walmsley@sifive.com>, <soha@lohu.info>,
        <twd2.me@gmail.com>
Subject: Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing
Message-ID: <20230427-related-generic-7573069edc67@wendy>
References: <20230427-unveiling-kiwi-631e966f77cc@wendy>
 <tencent_D9A598A810BDC656BE2BC15A6D0FC76C5C07@qq.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="kuwsqOfIl6S7aHip"
Content-Disposition: inline
In-Reply-To: <tencent_D9A598A810BDC656BE2BC15A6D0FC76C5C07@qq.com>
Precedence: bulk

--kuwsqOfIl6S7aHip
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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

>=20
> 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.
>=20
> > > also do a strlen(isa) >=3D 4 check first, though. of_property_read_st=
ring()
> > > will succeed even when the string is "".
>=20
> > 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.
>=20
> 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.

--kuwsqOfIl6S7aHip
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZEpHsAAKCRB4tDGHoIJi
0tVOAP9yB9QiDDkjzXlfN9sch23r/K4ll1odi/MQ+szxdGmt/gEA+VVbVqUJXUeS
mN+uJOVX0fufcXgSRjLlyNP/H4luEQA=
=O1Zd
-----END PGP SIGNATURE-----

--kuwsqOfIl6S7aHip--