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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Sep 12, 2024 at 07:44:05PM +0100, Andrew Cooper wrote:
> 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?

The problem is that (et->len / sizeof(et->eq[0])) - 1 will underflow,
causing Xen to access et->eq[UINT32_MAX], which will (hopefully) crash.
The simplest solution is to drop that check, since
scan_equiv_cpu_table() already checks for terminating entries.

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

Correct.  Anything nonzero will either be greater than MC_HEADER_SIZE or
will be rejected due to (total_size & 1023) not being zero.  That said,
the new code is much more obviously correct.

> 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

It's still undefined behavior (cast to an unaligned pointer).
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature