Re: [Xen-devel] [PATCH] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

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