| Message ID | 1566177928-19114-2-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve late microcode loading | expand |
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@intel.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
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
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@intel.com> >> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > >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
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c index 22fdeca..c185b5c 100644 --- 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; + + 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; + + return MIS_UCODE; } static int microcode_sanity_check(void *mc) @@ -243,31 +268,12 @@ static int get_matching_microcode(const void *mc, unsigned int cpu) { struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); const struct microcode_header_intel *mc_header = mc; - const struct extended_sigtable *ext_header; unsigned long total_size = get_totalsize(mc_header); - int ext_sigcount, i; - struct extended_signature *ext_sig; void *new_mc; - if ( microcode_update_match(cpu, mc_header, - mc_header->sig, mc_header->pf) ) - goto find; - - if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) ) + if ( microcode_update_match(mc, cpu) != NEW_UCODE ) return 0; - ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE; - ext_sigcount = ext_header->count; - ext_sig = (void *)ext_header + EXT_HEADER_SIZE; - for ( i = 0; i < ext_sigcount; i++ ) - { - if ( microcode_update_match(cpu, mc_header, - ext_sig->sig, ext_sig->pf) ) - goto find; - ext_sig++; - } - return 0; - find: pr_debug("microcode: CPU%d found a matching microcode update with" " version %#x (current=%#x)\n", cpu, mc_header->rev, uci->cpu_sig.rev); diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 23ea954..882f560 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -3,6 +3,12 @@ #include <xen/percpu.h> +enum microcode_match_result { + OLD_UCODE, /* signature matched, but revision id is older or equal */ + NEW_UCODE, /* signature matched, but revision id is newer */ + MIS_UCODE, /* signature mismatched */ +}; + struct cpu_signature; struct ucode_cpu_info;