Re: [PATCH] xen/ucode: Fix buffer under-run when parsing AMD containers

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/09/2024 1:45 pm, Jan Beulich wrote:
> On 13.09.2024 13:09, Andrew Cooper wrote:
>> From: Demi Marie Obenour <demi@invisiblethingslab.com>
>>
>> The AMD container format has no formal spec.  It is, at best, precision
>> guesswork based on AMD's prior contributions to open source projects.  The
>> Equivalence Table has both an explicit length, and an expectation of having a
>> NULL entry at the end.
>>
>> Xen was sanity checking the NULL entry, but without confirming that an entry
>> was present, resulting in a read off the front of the buffer.  With some
>> manual debugging/annotations this manifests as:
>>
>>   (XEN) *** Buf ffff83204c00b19c, eq ffff83204c00b194
>>   (XEN) *** eq: 0c 00 00 00 44 4d 41 00 00 00 00 00 00 00 00 00 aa aa aa aa
>>                             ^-Actual buffer-------------------^
>>   (XEN) *** installed_cpu: 000c
>>   (XEN) microcode: Bad equivalent cpu table
>>   (XEN) Parsing microcode blob error -22
>>
>> When loaded by hypercall, the 4 bytes interpreted as installed_cpu happen to
>> be the containing struct ucode_buf's len field, and luckily will be nonzero.
>>
>> When loaded at boot, it's possible for the access to #PF if the module happens
>> to have been placed on a 2M boundary by the bootloader.  Under Linux, it will
>> commonly be the end of the CPIO header.
>>
>> Drop the probe of the NULL entry; Nothing else cares.  A container without one
>> is well formed, insofar that we can still parse it correctly.  With this
>> dropped, the same container results in:
>>
>>   (XEN) microcode: couldn't find any matching ucode in the provided blob!
>>
>> Fixes: 4de936a38aa9 ("x86/ucode/amd: Rework parsing logic in cpu_request_microcode()")
>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> I wonder though about scan_equiv_cpu_table(): Should it perhaps complain
> if it doesn't find a null entry? And when it find ones, but that's not
> last?

I'm erring on the "be liberal on what you accept" side of things.

scan_equiv_cpu_table() is a relic of Fam10h systems (the last time that
two different CPU steppings shared a ucode blob), and recent changes to
Linux suggest that AMD have retired the concept.

The only thing we do in scan_equiv_cpu_table() is see if there's a
mapping for the current system, and we loop over all entries, even if
that happens to be 0.

~Andrew