linux-kernel - Re: [GIT pull] x86/irq for v5.18-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Mar 2022 12:17:07 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        "Maciej W. Rozycki" <macro@...am.me.uk>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [GIT pull] x86/irq for v5.18-rc1

On Mon, Mar 21, 2022 at 4:02 AM Thomas Gleixner <tglx@...utronix.de> 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/a2791312-2957-27e6-43af-c805bbb90266@collabora.com/

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