[v1] Fix two problems in the microcode parsers

[PATCH] Fix two problems in the microcode parsers

Posted by Demi Marie Obenour 1 year, 1 month ago

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.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 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 )
         {
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 )
     {
         printk(XENLOG_WARNING "microcode: Bad size\n");
         return -EINVAL;

base-commit: 035baa203b978b219828d0d3c16057beb344f35c
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Posted by Andrew Cooper 1 year, 1 month ago

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@invisiblethingslab.com>
> ---
>  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

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

Posted by Demi Marie Obenour 1 year, 1 month ago

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@invisiblethingslab.com>
> > ---
> >  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

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

Posted by Andrew Cooper 1 year, 1 month ago

On 12/09/2024 7:44 pm, Andrew Cooper wrote:
> On 12/09/2024 6:39 pm, Demi Marie Obenour wrote:
>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>> ---
>>  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.

Bah, and just after sending, I've spotted the problem.  When len is 0,
the installed_cpu check (which is looking for the NULL entry) moves off
the front of the buffer.

I'm very tempted to first wire this into OSS_FUZZ, seeing as we've just
got that working for the emulator.

~Andrew

[PATCH v2] Fix two problems in the microcode parsers

Posted by Demi Marie Obenour 1 year, 1 month ago

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.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/cpu/microcode/amd.c   | 6 +++---
 xen/arch/x86/cpu/microcode/intel.c | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d2a26967c6dbc4695602dd46d5836a6d88e15072..08fe3ac61c18a8e16f694e128973da96ce6995e3 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;
@@ -365,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;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 6f6957058684d7275d62e525e88ff678db9eb6d2..3e113c84b1fff0ba18a0251dbac0c7d6e2b229f6 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -149,8 +149,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
 {
     const struct extended_sigtable *ext;
     const uint32_t *ptr;
-    unsigned int total_size = get_totalsize(patch);
-    unsigned int data_size = get_datasize(patch);
+    uint32_t total_size = get_totalsize(patch);
+    uint32_t data_size = get_datasize(patch);
     unsigned int i, ext_size;
     uint32_t sum;
 
@@ -159,7 +159,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
      * must fit within it.
      */
     if ( (total_size & 1023) ||
-         data_size > (total_size - MC_HEADER_SIZE) )
+         data_size > (total_size - MC_HEADER_SIZE) ||
+         (data_size % 4) != 0 )
     {
         printk(XENLOG_WARNING "microcode: Bad size\n");
         return -EINVAL;

base-commit: 035baa203b978b219828d0d3c16057beb344f35c
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Posted by Roger Pau Monné 1 year, 1 month ago

On Thu, Sep 12, 2024 at 05:11:32PM -0400, 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.

As said by Jan I think this needs expanding as to what's actually
being fixed, to give readers context.

Additionally you want to add one or more "Fixes" tags if this is a
bugfix.

Thanks, Roger.

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

Posted by Jan Beulich 1 year, 1 month ago

On 12.09.2024 23:11, 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.

Okay, this explains the background. But nothing is said about what's
actually wrong with the code that's being changed.

As to the subject: Please have it have a proper prefix.

> --- 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;
> @@ -365,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 )

What's this literal 4? Wants to be some alignof(), sizeof(), or alike
imo.

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -149,8 +149,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
>  {
>      const struct extended_sigtable *ext;
>      const uint32_t *ptr;
> -    unsigned int total_size = get_totalsize(patch);
> -    unsigned int data_size = get_datasize(patch);
> +    uint32_t total_size = get_totalsize(patch);
> +    uint32_t data_size = get_datasize(patch);

I see no reason for moving in the wrong direction here, as far as
./CODING_STYLE goes. If this is truly needed,it needs justifying in
the description.

> @@ -159,7 +159,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
>       * must fit within it.
>       */
>      if ( (total_size & 1023) ||
> -         data_size > (total_size - MC_HEADER_SIZE) )
> +         data_size > (total_size - MC_HEADER_SIZE) ||
> +         (data_size % 4) != 0 )

See above as to the use of literal numbers.

Jan