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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2019 09:15, Chao Gao wrote:
> On Wed, Aug 28, 2019 at 05:12:34PM +0200, Jan Beulich wrote:
>> On 19.08.2019 03:25, Chao Gao wrote:
>>> --- 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.

Well - that's the main question. The purpose of this patch, after all,
is (aiui) to allow calling the function in more cases. If all callers
are indeed supposed to check the basic properties, then yes, an ASSERT()
would be fine.

Jan

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