linux-kernel - Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

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]

Message-ID: <b561524f3378fb1d4c69451fdbf70bb5cbbb5ec9.camel@intel.com>
Date:   Fri, 1 Dec 2023 18:08:55 +0000
From:   "Zhang, Rui" <rui.zhang@...el.com>
To:     "ashok_raj@...ux.intel.com" <ashok_raj@...ux.intel.com>
CC:     "linux-tip-commits@...r.kernel.org" 
        <linux-tip-commits@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jsperbeck@...gle.com" <jsperbeck@...gle.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tip-bot2@...utronix.de" <tip-bot2@...utronix.de>
Subject: Re: [tip: x86/urgent] x86/acpi: Ignore invalid x2APIC entries

On Thu, 2023-11-30 at 19:25 -0800, Ashok Raj wrote:
> On Thu, Nov 23, 2023 at 12:50:47PM +0000, Zhang Rui wrote:
> > Hi, John,
> > 
> > Thanks for catching this issue.
> > 
> > On Wed, 2023-11-22 at 22:19 +0000, John Sperbeck wrote:
> > > I have a platform with both LOCAL_APIC and LOCAL_X2APIC entries
> > > for
> > > each CPU.  However, the ids for the LOCAL_APIC entries are all
> > > invalid ids of 255, so they have always been skipped in
> > > acpi_parse_lapic()
> > > by this code from f3bf1dbe64b6 ("x86/acpi: Prevent LAPIC id 0xff
> > > from
> > > being
> > > accounted"):
> > > 
> > >     /* Ignore invalid ID */
> > >     if (processor->id == 0xff)
> > >             return 0;
> > > 
> > > With the change in this thread, the return value of 0 means that
> > > the
> > > 'count' variable in acpi_parse_entries_array() is incremented. 
> > > The
> > > positive return value means that 'has_lapic_cpus' is set, even
> > > though
> > > no entries were actually matched.
> > 
> > So in acpi_parse_madt_lapic_entries, without this patch,
> > madt_proc[0].count is a positive value on this platform, right?
> > 
> > This sounds like a potential issue because the following checks to
> > fall
> > back to MPS mode can also break. (If all LOCAL_APIC entries have
> > apic_id 0xff and all LOCAL_X2APIC entries have apic_id 0xffffffff)
> > 
> > >   Then, when the MADT is iterated
> > > with acpi_parse_x2apic(), the x2apic entries with ids less than
> > > 255
> > > are skipped and most of my CPUs aren't recognized.
> 
> This smells wrong. If a BIOS is placing some in lapic and some in
> x2apic
> table, its really messed up. 
> 
> Shouldn't the kernel scan them in some priority and only consider one
> set of
> tables?
> 
> Shouldn't the code stop looking once something once a type is found?
> 

I also want to get this clarified but there is no spec saying this. And
instead, as mentioned in the comment, we do have something in
https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure

"[Compatibility note] On some legacy OSes, Logical processors with APIC
ID values less than 255 (whether in XAPIC or X2APIC mode) must use the
Processor Local APIC structure to convey their APIC information to
OSPM, and those processors must be declared in the DSDT using the
Processor() keyword. Logical processors with APIC ID values 255 and
greater must use the Processor Local x2APIC structure and be declared
using the Device() keyword."

so it is possible to enumerate CPUs from both LAPIC and X2APIC.

thanks,
rui

> sapic is only for IA64 correct?
> 
> sapic_entries =
> acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,....)
> 
> if sapic_entries
>         return sapic_entries;
> 
> x2apic_count = ...
> 
> if x2apic_count
>         return x2apic_count;
> 
> apic_count = ...
> 
> Maybe you should add if all entries are marked INVALID for APIC, just
> ignore the whole table?
> 
> Cheers,
> Ashok