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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2024 13:09, Andrew Cooper wrote:
> From: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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?

Jan