[v2,6/7] x86/ucode/intel: Clean up microcode_sanity_check()
diff mbox series

Message ID 20200327122901.11569-7-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/ucode: Cleanup and fixes - Part 3/n (Intel)
Related show

Commit Message

Andrew Cooper March 27, 2020, 12:29 p.m. UTC
Rewrite the size checks in a way which doesn't depend on Xen being compiled as
64bit.

Introduce a check missing from the old code, that total_size is a multiple of
1024 bytes, and drop unnecessary defines/macros/structures.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over struct microcode_patch re-work
 * Retain constness on cast through void *
 * Reinstate printk()s for bad data
---
 xen/arch/x86/cpu/microcode/intel.c | 147 +++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 81 deletions(-)

Comments

Jan Beulich March 31, 2020, 2:18 p.m. UTC | #1
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. With this addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper March 31, 2020, 2:26 p.m. UTC | #2
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

Patch
diff mbox series

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 9d8d5bfc6e..1358a25032 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -69,24 +69,19 @@  struct microcode_patch {
 #define microcode_intel microcode_patch
 
 /* microcode format is extended from prescott processors */
-struct extended_signature {
-    unsigned int sig;
-    unsigned int pf;
-    unsigned int cksum;
-};
-
 struct extended_sigtable {
     unsigned int count;
     unsigned int cksum;
     unsigned int reserved[3];
-    struct extended_signature sigs[0];
+    struct {
+        unsigned int sig;
+        unsigned int pf;
+        unsigned int cksum;
+    } sigs[];
 };
 
 #define PPRO_UCODE_DATASIZE     2000
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
-#define EXT_HEADER_SIZE         (sizeof(struct extended_sigtable))
-#define EXT_SIGNATURE_SIZE      (sizeof(struct extended_signature))
-#define DWSIZE                  (sizeof(u32))
 
 static uint32_t get_datasize(const struct microcode_patch *patch)
 {
@@ -134,8 +129,6 @@  static bool signature_matches(const struct cpu_signature *cpu_sig,
     return cpu_sig->pf & ucode_pf;
 }
 
-#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
-
 static int collect_cpu_info(struct cpu_signature *csig)
 {
     uint64_t msr_content;
@@ -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;
-    }
-    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;
     }
 
-    /* check extended table checksum */
-    if ( ext_table_size )
+    /*
+     * Check the structure of the extended signature table, ensuring that it
+     * fits exactly in the remaining space.
+     */
+    ext = (const void *)&patch->data[data_size];
+    if ( ext_size < sizeof(*ext) ||
+         (ext_size - sizeof(*ext)) % sizeof(ext->sigs[0]) ||
+         (ext_size - sizeof(*ext)) / sizeof(ext->sigs[0]) != ext->count )
     {
-        uint32_t ext_table_sum = 0;
-        uint32_t *ext_tablep = (uint32_t *)ext_header;
-
-        i = ext_table_size / DWSIZE;
-        while ( i-- )
-            ext_table_sum += ext_tablep[i];
-        if ( ext_table_sum )
-        {
-            printk(KERN_WARNING "microcode: aborting, "
-                   "bad extended signature table checksum\n");
-            return -EINVAL;
-        }
+        printk(XENLOG_WARNING "microcode: Bad sigtable size\n");
+        return -EINVAL;
     }
 
-    /* calculate the checksum */
-    orig_sum = 0;
-    i = (MC_HEADER_SIZE + data_size) / DWSIZE;
-    while ( i-- )
-        orig_sum += ((uint32_t *)mc)[i];
-    if ( orig_sum )
+    /* Checksum the whole extended signature table. */
+    for ( sum = 0, ptr = (const uint32_t *)ext;
+          ptr < (const uint32_t *)&ext->sigs[ext->count]; ++ptr )
+        sum += *ptr;
+
+    if ( sum != 0 )
     {
-        printk(KERN_ERR "microcode: aborting, bad checksum\n");
+        printk(XENLOG_WARNING "microcode: Bad sigtable checksum\n");
         return -EINVAL;
     }
-    if ( !ext_table_size )
-        return 0;
-    /* check extended signature checksum */
-    for ( i = 0; i < ext_sigcount; i++ )
-    {
-        ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
-            EXT_SIGNATURE_SIZE * i;
-        sum = orig_sum
-            - (mc_header->sig + mc_header->pf + mc_header->cksum)
-            + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
-        if ( sum )
+
+    /*
+     * Checksum each indiviudal extended signature as if it had been in the
+     * main header.
+     */
+    sum = patch->hdr.sig + patch->hdr.pf + patch->hdr.cksum;
+    for ( i = 0; i < ext->count; ++i )
+        if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
         {
-            printk(KERN_ERR "microcode: aborting, bad checksum\n");
+            printk(XENLOG_WARNING "microcode: Bad sigtable checksum\n");
             return -EINVAL;
         }
-    }
+
     return 0;
 }