From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply Date: Fri, 25 Apr 2014 14:48:06 -0500 Message-ID: <535ABBF6.1060303@amd.com> References: <1398369287-2501-1-git-send-email-aravind.gopalakrishnan@amd.com> <5359738A.6050607@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5359738A.6050607@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Thomas.Lendacky@amd.com, keir@xen.org, Suravee.Suthikulpanit@amd.com, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 4/24/2014 3:26 PM, Andrew Cooper wrote: > On 24/04/14 20:54, Aravind Gopalakrishnan wrote: >> >> -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 Noted. >> +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. Right. Modified it to use XENLOG_DEBUG instead. >> >> 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? > Right. Didn't think about that. Sorry.. But I ran into larger issues here: Since we don't break on the first match and continue to parse the entire fw image till the very end; *and* since I modified the error handling around 'microcode_fits' thus: if ( (mc_header->processor_rev_id) != equiv_cpu_id ) -return 0; +return -EINVAL; + The last returned error val is '-22' which is bubbled up to microcode.c. 'microcode_presmp_init' as a result flushes out ucode_blob or NULL-ifies ucode_mod_map. Which means 'microcode_init' returns early as it can't validate ucode_mod.mod_end or ucode_blob.size Now, this breaks original functionality (sorry for catching this late), *but* doesn't cause any problem to updating(,applying) ucode patches to cpus as we apply the patches during 'microcode_resume_cpu' anyway. (which is why I thought at first all is fine) Could someone please help me understand why there are two initcalls - 'microcode_presmp_init' && 'microcode_init' that perform the same tasks? It results in these functions - 'collect_cpu_info', 'cpu_request_microcode' to run a second time through 'microcode_init' when we have already accomplished the job of updating cpu with microcode patches through 'microcode_presmp_init' and 'microcode_resume_cpu' If there is particular reason for 'microcode_init''s existence, then I'll modify the patch so that the error handling around 'microcode_fits' is not altered. But if not, then I simply have to just remove the 'break' statement. Thanks, -Aravind.