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

[Jan Beulich <jbeulich@xxxxxxxx>]

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