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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

Jan

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