Re: [Xen-devel] [PATCH v9 01/15] microcode/intel: extend microcode_update_match()

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Aug 28, 2019 at 05:12:34PM +0200, Jan Beulich wrote:
>On 19.08.2019 03:25, Chao Gao wrote:
>> to a more generic function. So that it can be used alone to check
>> an update against the CPU signature and current update revision.
>> 
>> Note that enum microcode_match_result will be used in common code
>> (aka microcode.c), it has been placed in the common header.
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
>I don't think these can be legitimately retained with ...
>
>> Changes in v9:
>>  - microcode_update_match() doesn't accept (sig, pf, rev) any longer.
>>  Hence, it won't be used to compare two arbitrary updates.
>
>... this kind of a change.

Will drop RBs.

>
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -134,14 +134,39 @@ 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)
>> +/* Check an update against the CPU signature and current update revision */
>> +static enum microcode_match_result microcode_update_match(
>> +    const struct microcode_header_intel *mc_header, unsigned int cpu)
>>  {
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> -
>> -    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>> -            (mc_header->rev > uci->cpu_sig.rev));
>> +    const struct extended_sigtable *ext_header;
>> +    const struct extended_signature *ext_sig;
>> +    unsigned int i;
>> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> +    unsigned int sig = uci->cpu_sig.sig;
>> +    unsigned int pf = uci->cpu_sig.pf;
>> +    unsigned int rev = uci->cpu_sig.rev;
>> +    unsigned long data_size = get_datasize(mc_header);
>> +    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;
>
>Didn't you lose a range check against "end" ahead of this if()?
>get_totalsize() and get_datasize() aiui also would need to live
>after a range check, just a sizeof() (i.e. MC_HEADER_SIZE) based
>one. This would also affect the caller as it seems.

I think microcode_sanity_check() is for this purpose. We can do
sanity check before the if(). Perhaps, we can just add an assertion
that sanity check won't fail. Because whenever sanity check failed
when pasing an ucode blob, we just drop the ucode; we won't pass an
broken ucode to this function.

Thanks
Chao

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