Re: [PATCH] Fix two problems in the microcode parsers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/2024 6:39 pm, Demi Marie Obenour wrote:
> The microcode might come from a questionable source, so it is necessary
> for the parsers to treat it as untrusted.  The CPU will validate the
> microcode before applying it, so loading microcode from unofficial
> sources is actually a legitimate thing to do in some cases.

While true, the better argument is that ucode blobs are mostly encrypted
data, so a mis-step tends to fall over a very long way.

>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/cpu/microcode/amd.c   | 1 +
>  xen/arch/x86/cpu/microcode/intel.c | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/amd.c 
> b/xen/arch/x86/cpu/microcode/amd.c
> index 
> d2a26967c6dbc4695602dd46d5836a6d88e15072..31ee5717c5f1c7d0b7e29d990cf4d1024d775900
>  100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -338,6 +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->len % sizeof(et->eq[0]) ||
>               et->eq[(et->len / sizeof(et->eq[0])) - 1].installed_cpu )
>          {

This is complicated by AMD having no spec in the slightest on their
container format, but this change makes Xen reject a case that I
reasoned was well formed (if unwise).

IMO, the following binary is well formed:

    0x00414d44 /* MAGIC */
    0x00000000 /* type=equiv */
    0x00000000 /* len=0, == no entries */
               /* no equiv entries */
               /* no ucode entries */
    0x00414d44 /* MAGIC */
    ...

and I believe that Xen would parse it correctly.  Unless you think
there's another case I've failed to consider?

AFACT the check you put in rejects the len < "1 entry" case.

There is no shortage of ambiguity here; the equiv table has both an
explicit length, and commonly has a NULL entry at the end.  But, we can
parse with a length of 0.


> diff --git a/xen/arch/x86/cpu/microcode/intel.c 
> b/xen/arch/x86/cpu/microcode/intel.c
> index 
> 6f6957058684d7275d62e525e88ff678db9eb6d2..7a383adbdf1b5cb58f2e4c89e3a1c11ecc053993
>  100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -158,8 +158,9 @@ static int microcode_sanity_check(const struct 
> microcode_patch *patch)
>       * Total size must be a multiple of 1024 bytes.  Data size and the header
>       * must fit within it.
>       */
> -    if ( (total_size & 1023) ||
> -         data_size > (total_size - MC_HEADER_SIZE) )
> +    if ( (total_size & 1023) || (total_size < MC_HEADER_SIZE) ||
> +         data_size > (total_size - MC_HEADER_SIZE) ||
> +         (data_size % 4) != 0 )

The caller has already checked some properties.

Furthermore, total_size of 0 in the header is a special case for
pre-Pentium4 ucode and is substituted with a constant, so can never be 0
in the variable here.  Therefore, I don't think the (total_size <
MC_HEADER_SIZE) check can ever fail.

data_size being 4-byte aligned probably should be checked, although I
think the later cross-check (ext_sigtable fits exactly in the remaining
space) should catch this before the function wanders off the buffer.

~Andrew