Re: [Xen-devel] [PATCH 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Rewrite the size checks in a way which which doesn't depend on Xen being
> compiled as 64bit.

One too many "which"?

> Introduce a check missing from the old code, that total_size is a multiple of
> 1024 bytes,

Where is this documented? The rather brief section in SDM vol 3 doesn't
mention anything like this.

> and drop unnecessarily defines/macros/structures.

unnecessary?

> @@ -160,93 +153,69 @@ static int collect_cpu_info(struct cpu_signature *csig)
>      return 0;
>  }
>  
> +/*
> + * Sanity check a blob which is expected to be a microcode patch.  The 48 
> byte
> + * header is of a known format, and together with totalsize are within the
> + * bounds of the container.  Everything else is unchecked.
> + */
>  static int microcode_sanity_check(const struct microcode_intel *mc)
>  {
> -    const struct microcode_header_intel *mc_header = &mc->hdr;
> -    const struct extended_sigtable *ext_header = NULL;
> -    const struct extended_signature *ext_sig;
> -    unsigned long total_size, data_size, ext_table_size;
> -    unsigned int ext_sigcount = 0, i;
> -    uint32_t sum, orig_sum;
> -
> -    total_size = get_totalsize(mc_header);
> -    data_size = get_datasize(mc_header);
> -    if ( (data_size + MC_HEADER_SIZE) > total_size )
> -    {
> -        printk(KERN_ERR "microcode: error! "
> -               "Bad data size in microcode data file\n");
> +    const struct extended_sigtable *ext;
> +    unsigned int total_size = get_totalsize(&mc->hdr);
> +    unsigned int data_size = get_datasize(&mc->hdr);
> +    unsigned int i, ext_size;
> +    uint32_t sum, *ptr;
> +
> +    /*
> +     * Total size must be a multiple of 1024 bytes.  Data size and the header
> +     * must fit within it.
> +     */
> +    if ( (total_size & 1023) ||

Personally I'd fine a hex number easier to recognize in cases like
this.

> +         data_size > (total_size - MC_HEADER_SIZE) )
>          return -EINVAL;
> -    }
>  
> -    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
> -    {

Ah - you're dropping this check here altogether. As said on the
earlier patch, I think this may more logically go there.

> -        printk(KERN_ERR "microcode: error! "
> -               "Unknown microcode update format\n");

While this printk() was already suggested to be moved, I'm not
convinced dropping others further down is helpful in case of
issues. We'd see just -EINVAL with no further indication of
what was (deemed) wrong.

> +    /* Checksum the main header and data. */
> +    for ( sum = 0, ptr = (uint32_t *)mc;
> +          ptr < (uint32_t *)&mc->data[data_size]; ++ptr )

You're casting away constness here which future compilers may
(legitimately) warn about. (Similarly again further down.)

Jan