RE: ACPI: regression: Failed to initialize GIC IRQ controller

[Moore, Robert]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx]
> Sent: Friday, July 10, 2015 10:03 AM
> To: Moore, Robert
> Cc: Ming Lei; Zheng, Lv; Wysocki, Rafael J; Linux Kernel Mailing List;
> linux-arm-kernel; Thomas Gleixner; Jason Cooper; hanjun.guo@xxxxxxxxxx
> Subject: Re: ACPI: regression: Failed to initialize GIC IRQ controller
> 
> On Fri, Jul 10, 2015 at 04:47:34PM +0100, Moore, Robert wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx]
> > > Sent: Friday, July 10, 2015 8:18 AM
> > > To: Moore, Robert
> > > Cc: Ming Lei; Zheng, Lv; Wysocki, Rafael J; Linux Kernel Mailing
> > > List; linux-arm-kernel; Thomas Gleixner; Jason Cooper;
> > > hanjun.guo@xxxxxxxxxx
> > > Subject: Re: ACPI: regression: Failed to initialize GIC IRQ
> > > controller
> > >
> > > On Fri, Jul 10, 2015 at 03:45:32PM +0100, Moore, Robert wrote:
> > > > It's nice that someone took a sizeof() on the struct -- so, one
> > > > would
> > > hope that no code actually depended on a particular value, no?
> > >
> > > Unfortunately that sizeof has been there forever (x86/ia64),
> > > ia64 code ran into a similar issue, so the check was removed to cope
> > > with lsapic MADT updates, see:
> > >
> > > arch/ia64/kernel/acpi.c line 204
> > >
> > > /*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
> > >
> > > Is checking the subtable length field against a static value really
> > > worthwhile/suitable ?
> > >
> >
> > I would at least traverse the subtables via the subtable length given in
> the table, and not use a sizeof() for each subtable. Then, multiple
> table/subtable versions are handled automatically; you don't have to use
> any new fields until necessary.
> 
> I lost you here, sorry. You are describing how the subtable entries are
> parsed in acpi_parse_entries, but that's not what we are debating here.
> BAD_MADT_ENTRY checks the subtable length against the ACPICA MADT structs
> sized through sizeof to determine if the length field is "correct", I do
> not see how you can do it by traversing the tables (how can you determine
> where a subtable _really_ ends or to put it differently how to check that
> a subtable length is _really_ right ?).

Ok, this is not the actual traversal.

However, the traversal probably uses the subtable length entries already, or it least it should.
I think that the subtable lengths are one field that absolutely *must* be trusted, else lots of things would break. Perhaps the MADT does not have variable-length subtable fields (I forget), but many ACPI tables do.

Once you've got a subtable, you want to probable check it it is at least as long as you expect. Any longer should be treated as OK, since it probably contains fields that you don't know how to decipher anyway.




> 
> Thanks,
> Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/