Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Sep 11, 2025 at 5:04 PM CEST, Jan Beulich wrote:
> On 11.09.2025 14:03, Andrew Cooper wrote:
>> On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
>>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>>> by the device model. The GPE handler checks this and compares it against
>>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>>> related bit in the bitmap and adjusting the table's checksum.
>>>
>>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>>> reaches 128, even if that overflows the MADT into some other (hopefully
>>> mapped) memory. The reading isn't as problematic as the writing though.
>>>
>>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>>> then the bit where the "online" flag would be is flipped, thus
>>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>>> that happened outside its range. It's all terrible.
>>>
>>> Note that this corruption happens regardless of the device-model being
>>> present or not, because even if the bitmap holds 0s, the overflowed
>>> memory might not at the bits corresponding to the "online" flag.
>>>
>>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>>
>>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>>> Reported-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
>>> ---
>>> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is 
>>> tested
>>> there.
>>>
>>> v2:
>>>   * New patch with the general fix for HVM too. Turns out the correction
>>>     logic was buggy after all.
>> 
>> Hmm, this does sound rather more serious.  I have a nagging feeling that
>> until recently we always wrote 128 MADT entries.
>
> Not exactly recently, but looks like that's my fault then: 0875433389240
> ("hvmloader: limit CPUs exposed to guests").
>
> Jan

Very right. I got to that commit, but thought nr_processor_objects would match 
NCPUS. Wrong assumption.

That sorts out wich fixes tag to attribute this to.

Cheers,
Alejandro