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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan