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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()
Date: Tue, 31 Mar 2020 15:26:40 +0100	[thread overview]
Message-ID: <2e740576-99a0-2010-2d8f-bc10ec556dc6@citrix.com> (raw)
In-Reply-To: <e6b7cd41-3669-640f-71ad-e868e9fd513a@suse.com>

On 31/03/2020 15:18, Jan Beulich wrote:
> On 27.03.2020 13:29, Andrew Cooper wrote:
>> @@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig)
>>      return 0;
>>  }
>>  
>> -static int microcode_sanity_check(const struct microcode_patch *mc)
>> +/*
>> + * 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_patch *patch)
>>  {
>> -    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);
>> -    data_size = get_datasize(mc);
>> -    if ( (data_size + MC_HEADER_SIZE) > total_size )
>> +    const struct extended_sigtable *ext;
>> +    const uint32_t *ptr;
>> +    unsigned int total_size = get_totalsize(patch);
>> +    unsigned int data_size = get_datasize(patch);
>> +    unsigned int i, ext_size;
>> +    uint32_t sum;
>> +
>> +    /*
>> +     * Total size must be a multiple of 1024 bytes.  Data size and the header
>> +     * must fit within it.
>> +     */
>> +    if ( (total_size & 1023) ||
>> +         data_size > (total_size - MC_HEADER_SIZE) )
>>      {
>> -        printk(KERN_ERR "microcode: error! "
>> -               "Bad data size in microcode data file\n");
>> +        printk(XENLOG_WARNING "microcode: Bad size\n");
>>          return -EINVAL;
>>      }
>>  
>> -    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
>> -    {
>> -        printk(KERN_ERR "microcode: error! "
>> -               "Unknown microcode update format\n");
>> +    /* Checksum the main header and data. */
>> +    for ( sum = 0, ptr = (const uint32_t *)patch;
>> +          ptr < (const uint32_t *)&patch->data[data_size]; ++ptr )
>> +        sum += *ptr;
>> +
>> +    if ( sum != 0 )
>>          return -EINVAL;
> The error message for this looks to have been lost, or ...
>
>> -    }
>> -    ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
>> -    if ( ext_table_size )
>> +
>> +    /* Look to see if there is an extended signature table. */
>> +    ext_size = total_size - data_size - MC_HEADER_SIZE;
>> +
>> +    /* No extended signature table?  All done. */
>> +    if ( ext_size == 0 )
>>      {
>> -        if ( (ext_table_size < EXT_HEADER_SIZE) ||
>> -             ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) )
>> -        {
>> -            printk(KERN_ERR "microcode: error! "
>> -                   "Small exttable size in microcode data file\n");
>> -            return -EINVAL;
>> -        }
>> -        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
>> -        if ( ext_table_size != exttable_size(ext_header) )
>> -        {
>> -            printk(KERN_ERR "microcode: error! "
>> -                   "Bad exttable size in microcode data file\n");
>> -            return -EFAULT;
>> -        }
>> -        ext_sigcount = ext_header->count;
>> +        printk(XENLOG_WARNING "microcode: Bad checksum\n");
>> +        return 0;
> ... to have got mistakenly moved here.

It was mistakenly moved.  I found and fixed that at some point after
sending this series.

>  With this addressed
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew