Re: [Xen-devel] [PATCH v7 02/10] microcode/intel: extend microcode_update_match()

[Chao Gao <chao.gao@xxxxxxxxx>]

On Tue, Jun 04, 2019 at 08:39:15AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, 
>> struct cpu_signature *csig)
>>      return 0;
>>  }
>>  
>> -static inline int microcode_update_match(
>> -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>> -    int sig, int pf)
>> +static enum microcode_match_result microcode_update_match(
>> +    const struct microcode_header_intel *mc_header, unsigned int sig,
>> +    unsigned int pf, unsigned int rev)
>>  {
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> +    const struct extended_sigtable *ext_header;
>> +    const struct extended_signature *ext_sig;
>> +    unsigned long data_size = get_datasize(mc_header);
>> +    unsigned int i;
>> +
>> +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>> +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>As indicated before, I think you would better also provide an "equal"
>indication. Iirc I've told you that I have one system where the cores
>get handed over from the BIOS in an inconsistent state (only core
>has ucode loaded). Hence we'd want to be able to also _store_
>ucode matching that found on CPU 0, without actually want to _load_
>it there.

Will do. What if no microcode update is provided in this case? Shall
we refuse to boot? If we allow different microcode revisions in the
system, it would complicate late microcode loading.

>
>> -    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>> -            (mc_header->rev > uci->cpu_sig.rev));
>> +    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
>> +        return MIS_UCODE;
>
>Okay, you're tightening the original <= to == here. But if you're
>already tightening things, why don't you make sure you actually
>have enough data to ...
>
>> +    ext_header = (const void *)(mc_header + 1) + data_size;
>
>... hold an extended header, and then also to hold ...
>
>> +    ext_sig = (const void *)(ext_header + 1);
>> +    for ( i = 0; i < ext_header->count; i++ )
>> +        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>> +            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>... enough array elements?

Do you think below incremental change is fine?

diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 94a1561..3dcbd28 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -138,18 +138,25 @@ static enum microcode_match_result microcode_update_match(
     const struct extended_signature *ext_sig;
     unsigned long data_size = get_datasize(mc_header);
     unsigned int i;
+    const void *end = (const void *)mc_header + get_totalsize(mc_header);
 
     if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
         return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
-    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
-        return MIS_UCODE;
-
     ext_header = (const void *)(mc_header + 1) + data_size;
     ext_sig = (const void *)(ext_header + 1);
-    for ( i = 0; i < ext_header->count; i++ )
-        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
-            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+    /*
+     * Make sure there is enough space to hold an extended header and enough
+     * array elements.
+     */
+    if ( (end >= (const void *)ext_sig) &&
+         (end >= (const void *)(ext_sig + ext_header->count)) )
+    {
+        for ( i = 0; i < ext_header->count; i++ )
+            if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
+                return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+    }
 
     return MIS_UCODE;
 }

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel