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
