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
