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:10 -0500 Message-ID: <535ABBFA.7020104@amd.com> References: <1398369287-2501-1-git-send-email-aravind.gopalakrishnan@amd.com> <535A2418020000780000C255@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <535A2418020000780000C255@nat28.tlf.novell.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: Jan Beulich Cc: Thomas.Lendacky@amd.com, keir@xen.org, Suravee.Suthikulpanit@amd.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 4/25/2014 2:00 AM, Jan Beulich wrote: >>>> On 24.04.14 at 21:54, wrote: >> Each family has a stipulated max patch_size. Use this as >> additional sanity check before we apply it. > And iirc there was a size limit check earlier, and it got dropped for one > reason or another - did you check history? Yes, I believe you are referring to this commit: commit 5663cc8258cef27509a437ebd95061b8b01b9c01 Author: Christoph Egger AuthorDate: Thu Dec 15 11:00:09 2011 +0100 Commit: Christoph Egger CommitDate: Thu Dec 15 11:00:09 2011 +0100 x86/ucode: fix for AMD Fam15 CPUs Remove hardcoded maximum size a microcode patch can have. This is dynamic now. The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated. Signed-off-by: Christoph Egger Signed-off-by: Jan Beulich Committed-by: Jan Beulich The above patch was to make the microcode patch buffer allocation dynamic. The hunk below simply verifies that we don't exceed the 'max_size'.. >> @@ -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) >> + { >> + 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; > Please simply "return patch_size <= max_size" in cases like this. Okay. >> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) >> >> last_offset = offset; >> >> + if ( error == -EEXIST ) { > Coding style, but as Andrew already indicated this block of code isn't > correct anyway. Yes, will fix this. But some help in understanding the microcode_init calls would help me here.. (Pleas refer reply to Andrew's comments) >> @@ -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; > Is it really correct for this to get switched from success to error return? Previously, microcode_fits returned '1' for Success, '0' for error. So, the condition returns '0' when return val is '0' This mechanism is still preserved in the changes made above.. >> @@ -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; > Bogus (pointless) change? > >> @@ -408,7 +456,7 @@ err2: >> err1: >> xfree(mc_amd); >> uci->mc.mc_amd = NULL; >> - return -ENOMEM; >> + return error; > Same here. > Hmm. Okay, Will just revert this. Thanks, -Aravind.