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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Sep 13, 2024 at 12:09:07PM +0100, 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>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> 
> Split out of joint patch, and analyse.
> 
> I couldn't trigger any of the sanitisers with this, hence the manual
> debugging.
> 
> This patch intentionally doesn't include patch 2's extra hunk changing:
> 
>   @@ -364,7 +364,8 @@ static struct microcode_patch *cf_check 
> cpu_request_microcode(
>                if ( size < sizeof(*mc) ||
>                     (mc = buf)->type != UCODE_UCODE_TYPE ||
>                     size - sizeof(*mc) < mc->len ||
>   -                 mc->len < sizeof(struct microcode_patch) )
>   +                 mc->len < sizeof(struct microcode_patch) ||
>   +                 mc->len % 4 != 0 )
>                {
>                    printk(XENLOG_ERR "microcode: Bad microcode data\n");
>                    error = -EINVAL;
> 
> Intel have a spec saying the length is mutliple of 4.  AMD do not, and have
> microcode which genuinely isn't a multiple of 4.

In this case the structs at the top should be __attribute__((packed)) to
avoid undefined behavior.  That can be a separate patch, though.

> ---
>  xen/arch/x86/cpu/microcode/amd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/amd.c 
> b/xen/arch/x86/cpu/microcode/amd.c
> index d2a26967c6db..32490c8b7d2a 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -338,8 +338,7 @@ static struct microcode_patch *cf_check 
> cpu_request_microcode(
>          if ( size < sizeof(*et) ||
>               (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>               size - sizeof(*et) < et->len ||
> -             et->len % sizeof(et->eq[0]) ||
> -             et->eq[(et->len / sizeof(et->eq[0])) - 1].installed_cpu )
> +             et->len % sizeof(et->eq[0]) )
>          {
>              printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
>              error = -EINVAL;
> -- 
> 2.39.2
> 

Reviewed-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature