On Mon, Mar 21, 2022 at 4:02 AM Thomas Gleixner <[email protected]> wrote:
>
> - Handle the IRT routing table format in AMI BIOSes correctly
*Very* minor nit here in the hope of future cleanups: the other x86
irq routing table structions (Christ, that's a sentence that shouldn't
exist in a sane world) use "__attribute__((packed))" and this one uses
"__packed".
They are all right next to each other, maybe they could be made to
have the same syntax?
HOWEVER.
That's not what the problem with this pull is.
I pulled this and then I unpulled it.
Because that stupid IRT routing table code already been reported to cause bugs:
https://lore.kernel.org/all/[email protected]/
which seems to be because the $IRT signature check is complete garbage:
> + for (addr = (u8 *)__va(0xf0000); addr < (u8 *)__va(0x100000); addr++) {
> + rt = pirq_convert_irt_table(addr);
> + if (rt)
> + return rt;
The above doesn't seem like it could really ever have been tested
properly, since it will walk off the end of that __va(0x100000)
address: it will walk every byte up to the 1MB physical address, and
it will try to find that $IRT signature there, but if it never finds
it, IT WILL CHECK THE SIGNATURE PAST THE 1MB mark!
So I refuse to pull this, and it should never have been sent to me,
considering that it had a known bug, and it took me only moments to
see how completely wrong that code was.
The fix seems obvious (you don't walk every byte to 1M, you walk to 1M
- the size of the struct, and then you also check that the number of
entries actually fits - Dmitry can presumably test), but no way do I
want to get this kind of clearly broken thing this merge window.
And yes, I'm unhappy. This bug was reported a week ago. This should
not have been sent to me today.
I also assume and suspect that the $IRT format isn't even used in
modern PC's, so this must be some really odd special legacy case that
very few people can care about. No?
Linus
On Mon, Mar 21 2022 at 12:17, Linus Torvalds wrote:
> On Mon, Mar 21, 2022 at 4:02 AM Thomas Gleixner <[email protected]> wrote:
> I pulled this and then I unpulled it.
>
> Because that stupid IRT routing table code already been reported to cause bugs:
>
> https://lore.kernel.org/all/[email protected]/
I've missed that. My bad...
Thanks,
tglx
On Mon, 21 Mar 2022, Linus Torvalds wrote:
> Because that stupid IRT routing table code already been reported to cause bugs:
>
> https://lore.kernel.org/all/[email protected]/
>
> which seems to be because the $IRT signature check is complete garbage:
>
> > + for (addr = (u8 *)__va(0xf0000); addr < (u8 *)__va(0x100000); addr++) {
> > + rt = pirq_convert_irt_table(addr);
> > + if (rt)
> > + return rt;
>
> The above doesn't seem like it could really ever have been tested
> properly, since it will walk off the end of that __va(0x100000)
> address: it will walk every byte up to the 1MB physical address, and
> it will try to find that $IRT signature there, but if it never finds
> it, IT WILL CHECK THE SIGNATURE PAST THE 1MB mark!
Drat! I did verify this code in a simulated environment that does supply
a $IRT table (for a reporter who has an actual system; I'm not lucky
enough to have one), however somehow I didn't think of verifying it with a
setup that has neither a $PIR nor a $IRT table. Therefore this issue has
slipped ($PIR scanner works in 16-byte intervals, so it escapes the range
overrun), and then of course things started moving only while I am away
enjoying Italian mountains. Oh well, nobody's perfect.
Thanks for narrowing this down, I'll post a fixed version on or shortly
after this coming weekend. And sorry for the mess-up!
Maciej
On Mon, 21 Mar 2022, Linus Torvalds wrote:
> > - Handle the IRT routing table format in AMI BIOSes correctly
>
> *Very* minor nit here in the hope of future cleanups: the other x86
> irq routing table structions (Christ, that's a sentence that shouldn't
> exist in a sane world) use "__attribute__((packed))" and this one uses
> "__packed".
I have reviewed and reverified the code for resubmission now and frankly
I don't know where this "__packed" artefact has come from. I certainly
have "__attribute__((packed))" in all my copies of the change including
one I have submitted (though `checkpatch.pl' does want it indeed to be
`__packed' instead).
Also accessing memory beyond __va(0x100000) does not appear to crash on
my 32-bit x86 machine, so it must be something specific to x86-64. Not an
excuse for a range overrun of course, but still odd (and as I previously
mentioned I find it odd too that this code is ever run for x86-64 in the
first place).
Finally, following your suggestion I have added verification for a range
overrun for the whole table for both the existing $PIR format and the new
$IRT format. It isn't a big deal and we shouldn't trust external sources
of data.
Maciej