[PATCH v1 1/2] RISC-V: skip parsing multi-letter extensions starting with caps

[Andrew Jones]

On Wed, Apr 26, 2023 at 03:37:58PM +0100, Conor Dooley wrote:
> On Wed, Apr 26, 2023 at 03:54:55PM +0200, Andrew Jones wrote:
> > On Wed, Apr 26, 2023 at 03:08:25PM +0200, Andrew Jones wrote:
> > > On Wed, Apr 26, 2023 at 01:47:39PM +0100, Conor Dooley wrote:
> > > > On Wed, Apr 26, 2023 at 02:18:52PM +0200, Andrew Jones wrote:
> > > > > On Wed, Apr 26, 2023 at 11:43:24AM +0100, Conor Dooley wrote:
> > > > > > Yangyu Chen reported that if an multi-letter extension begins with a
> > > > > > capital letter the parser will treat the remainder of that multi-letter
> > > > > > extension as single-letter extensions.
> > > > > 
> > > > > I think the problem is that the parser doesn't completely abort when
> > > > > it sees something it doesn't understand. Continuing is risky since
> > > > > it may be possible to compose an invalid string that gets the parser
> > > > > to run off the rails.
> > > > 
> > > > Usually I am of the opinion that we should not seek the validate the dt
> > > > in the kernel, since there are tools for doing so *cough* dt-validate
> > > > *cough*. This one seemed like low hanging fruit though, since the parser
> > > > handles having capital letters in any of the other places after the
> > > > rv##, but falls over pretty badly for this particular issue.
> > > > 
> > > > In general, I don't think we need to be concerned about anything that
> > > > fails dt-validate though, you kinda need to trust that that is correct.
> > > > I'd argue that we might even do too much validation in the parser at
> > > > present.
> > > > Is there some attack vector, or ACPI related consideration, that I am
> > > > unaware of that makes this risky?
> > 
> > A bit unrelated to this, but your mention of ACPI made me go look at the
> > approved ECR[1] again for the ISA string. It says "Null-terminated ASCII
> > Instruction Set Architecture (ISA) string for this hart. The format of the
> > ISA string is defined in the RISC-V unprivileged specification." I suppose
> > we can still add additional requirements to an ACPI ISA string which the
> > Linux kernel will parse, but it'll be odd to point people at the DT
> > binding to do that. Maybe we should consider making the parser more
> > complete, possibly by importing it from some reference implementation or
> > something.
> 
> Heh, I wonder are we heading for some divergence here then. riscv,isa in
> a DT is explicitly *not* a match for that due to the
> backwards-incompatible changes made by RVI to extension definitions
> since riscv,isa was added to the dt-binding. Clarifying that one is the
> next patch in my todo list..
> 
> ACPI naively saying "it matches the spec" is asking for trouble, since
> there does not actually appear to be any sort of clarification about
> which *version* of the spec that may be. At least in the dt-binding, we
> have a format there, what happens to the ACPI spec if RVI decides that -
> is a suitable alternative to _ in some future edition? I don't think
> such a thing is all that likely, but surely you'd like to insulate the
> ABI from that sort of eventuality?

Agreed. I'll raise this concern with the ACPI people.

> 
> Perhaps the thing to do is to actually take Yangyu's first patch and my
> second one, since the problem with backwards compatibility doesn't stop
> the kernel from being more permissive?

I guess so? Every time you wake up and see a parser, you get six more
weeks of Winter.

drew