Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()

[Jan Beulich <JBeulich@xxxxxxxx>]

On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -134,14 +134,35 @@ 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);
> -
> -    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 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;

Both here and ...

> +    ext_header = (const void *)(mc_header + 1) + data_size;
> +    ext_sig = (const void *)(ext_header + 1);
> +
> +    /*
> +     * 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)) )
> +        return MIS_UCODE;
> +
> +    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;

... here there's again an assumption that there's strict ordering
between blobs, but as mentioned in reply to a later patch of an
earlier version this isn't the case. Therefore the function can't
be used to compare two arbitrary blobs, it may only be used to
compare a blob with what is already loaded into a CPU. I think it
is quite important to mention this restriction in a comment ahead
of the function.

The code itself looks fine to me, and a comment could perhaps be
added while committing; with such a comment
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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