[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86, amd_ucode: Verify max allowed patch size before apply
On 24/04/14 20:54, Aravind Gopalakrishnan wrote: > Each family has a stipulated max patch_size. Use this as > additional sanity check before we apply it. > > Also, if we find current patch level equal or higher, then we > return early from microcode_fits, but we don't do anything about it. > This means we waste a bit of boot time needlessly parsing remainder > of firmware image. > > This patch returns EEXIST when above situation occurs so that- > If we do apply patch during presmp_init, then we save bit of time > when these routines are invoked during microcode_init. > If there is nothing to apply during presmp_init, then we flush > out ucode_blob.size and ucode_blob.data anyway since we are > already at required level. > > Since 'microcode_fits' returns int now, modify error machinery > to return EINVAL for error and '0' for success. > > While at it, fix comment at very top to indicate we support ucode > patch loading from fam10h and higher. > > Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx> > Reviewed-by: Tom Lendacky <Thomas.Lendacky@xxxxxxx> > Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx> > --- > xen/arch/x86/microcode_amd.c | 70 > +++++++++++++++++++++++++++++++++++------- > 1 file changed, 59 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c > index b227173..6fafe85 100644 > --- a/xen/arch/x86/microcode_amd.c > +++ b/xen/arch/x86/microcode_amd.c > @@ -8,7 +8,7 @@ > * Tigran Aivazian <tigran@xxxxxxxxxxxxxxxxxxxx> > * > * This driver allows to upgrade microcode on AMD > - * family 0x10 and 0x11 processors. > + * family 0x10 and later. > * > * Licensed unter the terms of the GNU General Public > * License version 2. See file COPYING for details. > @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature > *csig) > return 0; > } > > -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) > +static bool_t verify_patch_size(uint32_t patch_size) > +{ > + uint8_t family; > + uint32_t max_size; > + > +#define F1XH_MPB_MAX_SIZE 2048 > +#define F14H_MPB_MAX_SIZE 1824 > +#define F15H_MPB_MAX_SIZE 4096 > +#define F16H_MPB_MAX_SIZE 3458 > + > + family = boot_cpu_data.x86; > + switch (family) You can drop the family variable and just switch on boot_cpu_data.x86 > + { > + case 0x14: > + max_size = F14H_MPB_MAX_SIZE; > + break; > + case 0x15: > + max_size = F15H_MPB_MAX_SIZE; > + break; > + case 0x16: > + max_size = F16H_MPB_MAX_SIZE; > + break; > + default: > + max_size = F1XH_MPB_MAX_SIZE; > + break; > + } > + > + if ( patch_size > max_size ) > + return 0; > + > + return 1; > +} > + > +static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) > { > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > const struct microcode_header_amd *mc_header = mc_amd->mpb; > @@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct microcode_amd > *mc_amd, int cpu) > } > > if ( !equiv_cpu_id ) > - return 0; > + return -EINVAL; > > if ( (mc_header->processor_rev_id) != equiv_cpu_id ) > - return 0; > + return -EINVAL; > + > + if ( !verify_patch_size(mc_amd->mpb_size) ) > + { > + printk(KERN_ERR "microcode: patch size mismatch\n"); > + return -EINVAL; XENLOG_ERR "microcode patch size too large" return -E2BIG; And is this really worth being an error as opposed to a warning, or frankly even just debug? It is presumably one of N possible blobs in the hypercall. > + } > > if ( mc_header->patch_id <= uci->cpu_sig.rev ) > - return 0; > + return -EEXIST; > > printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " > "update with version %#x (current=%#x)\n", > cpu, mc_header->patch_id, uci->cpu_sig.rev); > > - return 1; > + return 0; > } > > static int apply_microcode(int cpu) > @@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const void > *buf, size_t bufsize) > while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > &offset)) == 0 ) > { > - if ( microcode_fits(mc_amd, cpu) ) > + error = microcode_fits(mc_amd, cpu); > + if ( !error ) > { > error = apply_microcode(cpu); > if ( error ) > @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void > *buf, size_t bufsize) > > last_offset = offset; > > + if ( error == -EEXIST ) { > + printk(KERN_DEBUG "microcode: patch is already at required level > or greater.\n"); > + break; You can't break from the loop here. What if a subsequent blob in the buffer contains a newer piece of microcode? ~Andrew > + } > + > if ( offset >= bufsize ) > break; > } > @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void > *mc) > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > struct microcode_amd *mc_amd = uci->mc.mc_amd; > const struct microcode_amd *src = mc; > + int error; > > - if ( !microcode_fits(src, cpu) ) > - return 0; > + error = microcode_fits(src, cpu); > + if ( error ) > + return error; > > if ( src != mc_amd ) > { > @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void > *mc) > xfree(mc_amd); > } > > + error = -ENOMEM; > mc_amd = xmalloc(struct microcode_amd); > uci->mc.mc_amd = mc_amd; > if ( !mc_amd ) > - return -ENOMEM; > + return error; > mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size); > if ( !mc_amd->equiv_cpu_table ) > goto err1; > @@ -408,7 +456,7 @@ err2: > err1: > xfree(mc_amd); > uci->mc.mc_amd = NULL; > - return -ENOMEM; > + return error; > } > > static int start_update(void) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |