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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2025 13:53, 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...")

The code in question originates from e5dc62c4d4f1 ("hvmloader: Fix CPU
hotplug notify handler in ACPI DSDT"), though. Before that there was a
different issue (as mentioned in the description).

> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -239,7 +239,8 @@ int main(int argc, char **argv)
>          /* Extract current CPU's status: 0=offline; 1=online. */
>          stmt("And", "Local1, 1, Local2");
>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... 
> */
> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
> +        push_block("If", "And(LLess(%d, NCPU), LNotEqual(Local2, 
> \\_SB.PR%02X.FLG))",
> +                   cpu, cpu);

Don't we need to use \\_SB.NCPU here? From the other two uses it's not
quite clear; it might also be that the one using this form is actually
needlessly doing so. Yet here it may be better if only for consistency's
sake, as the LNotEqual() also operates on an absolute reference.

The other thing is that I'm not fluent in AML operand evaluation rules.
We want to avoid even the read access to FLG, and I'm unconvinced And()
will avoid evaluating its 2nd argument when the first one is 0. IOW this
may need to become a 2nd "If".

I further think that strictly speaking you mean LAnd() here, not And()
(but the above concern remains; all the ASL spec says is "Source1 and
Source2 are evaluated as integers" for both And() and LAnd()).

Jan