cpu_request_microcode() doesn't know the buffer is at least 4 bytes long
before inspecting UCODE_MAGIC.

install_equiv_cpu_table() doesn't know the boundary of the buffer it is
interpreting as an equivalency table.  This case was clearly observed at one
point in the past, given the subsequent overrun detection, but without
comprehending that the damage was already done.

Make the logic consistent with container_fast_forward() and pass size_left in
to install_equiv_cpu_table().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 6bf3a054d3..796745e928 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -303,11 +303,20 @@ static int get_ucode_from_buffer_amd(
 static int install_equiv_cpu_table(
     struct microcode_amd *mc_amd,
     const void *data,
+    size_t size_left,
     size_t *offset)
 {
-    const struct mpbhdr *mpbuf = data + *offset + 4;
+    const struct mpbhdr *mpbuf;
     const struct equiv_cpu_entry *eq;
 
+    if ( size_left < (sizeof(*mpbuf) + 4) ||
+         (mpbuf = data + *offset + 4,
+          size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
+    {
+        printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
+        return -EINVAL;
+    }
+
     *offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
 
     if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
@@ -417,7 +426,8 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
 
     current_cpu_id = cpuid_eax(0x00000001);
 
-    if ( *(const uint32_t *)buf != UCODE_MAGIC )
+    if ( bufsize < 4 ||
+         *(const uint32_t *)buf != UCODE_MAGIC )
     {
         printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
         error = -EINVAL;
@@ -447,24 +457,13 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
      */
     while ( offset < bufsize )
     {
-        error = install_equiv_cpu_table(mc_amd, buf, &offset);
+        error = install_equiv_cpu_table(mc_amd, buf, bufsize - offset, &offset);
         if ( error )
         {
             printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
             break;
         }
 
-        /*
-         * Could happen as we advance 'offset' early
-         * in install_equiv_cpu_table
-         */
-        if ( offset > bufsize )
-        {
-            printk(KERN_ERR "microcode: Microcode buffer overrun\n");
-            error = -EINVAL;
-            break;
-        }
-
         if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
                                &equiv_cpu_id) )
             break;
-- 
2.11.0

On 28.03.2020 16:29, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -303,11 +303,20 @@ static int get_ucode_from_buffer_amd(
>  static int install_equiv_cpu_table(
>      struct microcode_amd *mc_amd,
>      const void *data,
> +    size_t size_left,
>      size_t *offset)
>  {
> -    const struct mpbhdr *mpbuf = data + *offset + 4;
> +    const struct mpbhdr *mpbuf;
>      const struct equiv_cpu_entry *eq;
>  
> +    if ( size_left < (sizeof(*mpbuf) + 4) ||
> +         (mpbuf = data + *offset + 4,
> +          size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )

This proliferation of literal 4 made me look into where this 4
is coming from and why it's here. Afaict it's covering for
UCODE_MAGIC. There's no good sizeof() that could be used instead,
so how about getting rid of this addend altogether by setting
offset to sizeof(uint32_t) near

    if ( *(const uint32_t *)buf != UCODE_MAGIC )

in cpu_request_microcode() and adding sizeof(header[0]) into
*offset near

        if ( header[0] == UCODE_MAGIC &&
             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )

in container_fast_forward()? (The code following the big, 7-
bullet-point comment in cpu_request_microcode() may also
need adjustment, but I wonder if we couldn't better get rid
of it altogether.)

However, since the change as is looks correct to me and doesn't
make the situation much worse, I'm not objecting to you also
putting it in as is, in which case
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On 30/03/2020 12:44, Jan Beulich wrote:
> On 28.03.2020 16:29, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu/microcode/amd.c
>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>> @@ -303,11 +303,20 @@ static int get_ucode_from_buffer_amd(
>>  static int install_equiv_cpu_table(
>>      struct microcode_amd *mc_amd,
>>      const void *data,
>> +    size_t size_left,
>>      size_t *offset)
>>  {
>> -    const struct mpbhdr *mpbuf = data + *offset + 4;
>> +    const struct mpbhdr *mpbuf;
>>      const struct equiv_cpu_entry *eq;
>>  
>> +    if ( size_left < (sizeof(*mpbuf) + 4) ||
>> +         (mpbuf = data + *offset + 4,
>> +          size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
> This proliferation of literal 4 made me look into where this 4
> is coming from and why it's here. Afaict it's covering for
> UCODE_MAGIC.

Correct

> There's no good sizeof() that could be used instead,
> so how about getting rid of this addend altogether by setting
> offset to sizeof(uint32_t) near
>
>     if ( *(const uint32_t *)buf != UCODE_MAGIC )
>
> in cpu_request_microcode() and adding sizeof(header[0]) into
> *offset near
>
>         if ( header[0] == UCODE_MAGIC &&
>              header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
>
> in container_fast_forward()?

I am in the process of rewriting all of this.  I'm not entirely
convinced of the correctness of cpu_request_microcode() in the first
place, but I also can't find a concrete problem with it.

>  (The code following the big, 7-
> bullet-point comment in cpu_request_microcode() may also
> need adjustment, but I wonder if we couldn't better get rid
> of it altogether.)

I'm unconvinced by what is described there. It honestly seems easier to
scan to the end of all containers.

>
> However, since the change as is looks correct to me and doesn't
> make the situation much worse, I'm not objecting to you also
> putting it in as is, in which case
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.  I'm tempted to keep it as-is because I think it is rather more
amenable to backport.

However, given the rate of finding issues, I'm going to hold off on
committing this until I've completed the rewrite, and am fairly sure
there are no further issues lurking.

~Andrew