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

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

[Andrew Cooper]

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Demi Marie Obenour <demi@invisiblethingslab.com>

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.
---
 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

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

[Jan Beulich]

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>

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

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

[Andrew Cooper]

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

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

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 4031 bytes --]

On Fri, Sep 13, 2024 at 12:09:07PM +0100, 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>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> 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@invisiblethingslab.com>
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]