From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH V2] x86, amd_ucode: Verify max allowed patch size before apply Date: Tue, 29 Apr 2014 16:33:45 -0500 Message-ID: <53601AB9.8000907@amd.com> References: <1398702918-12685-1-git-send-email-aravind.gopalakrishnan@amd.com> <535F78A6020000780000D340@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: <535F78A6020000780000D340@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 , andrew.cooper3@citrix.com, xen-devel@lists.xen.org, keir@xen.org, Boris Ostrovsky Cc: Thomas.Lendacky@amd.com, Suravee.Suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org On 4/29/2014 3:02 AM, Jan Beulich wrote: >>>> On 28.04.14 at 18:35, wrote: >> +static bool_t verify_patch_size(uint32_t patch_size) >> +{ >> + 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 > Out of mere curiosity - what makes these numbers this odd? The last > one isn't even divisible by 4. I don't know.. (+Boris) : any ideas? >> @@ -123,8 +151,17 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) >> if ( (mc_header->processor_rev_id) != equiv_cpu_id ) >> return 0; >> >> + if ( !verify_patch_size(mc_amd->mpb_size) ) >> + { >> + printk(XENLOG_DEBUG "microcode: patch size mismatch\n"); >> + return -E2BIG; >> + } >> + >> if ( mc_header->patch_id <= uci->cpu_sig.rev ) >> - return 0; >> + { >> + printk(XENLOG_DEBUG "microcode: patch is already at required level or greater.\n"); >> + return -EEXIST; >> + } >> >> printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " >> "update with version %#x (current=%#x)\n", > Honestly I'm rather hesitant to accept further generally useless > messages, no matter that they get printed at KERN_DEBUG only. I'd > much rather see these as well as the existing ones to be converted > to pr_debug(), thus easily enabled if someone really needs to do > debugging here. That's mainly because I (and I suppose other > developers do so to) try to run with loglvl=all wherever possible, > yet already on the 2x4-core box (not to speak of the newer 2x12- > core one) I find these rather annoying. Hmm, Okay. I'll work on this and send an updated version.. > And btw - now that you switch microcode_fits() back to returning > int (and -errno values) you would need to alter other return paths > to. That said, I don't really see why this return type change is > needed, the more that ... > >> @@ -319,7 +356,7 @@ 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) ) >> + if ( (error = microcode_fits(mc_amd, cpu)) > 0 ) >> { >> error = apply_microcode(cpu); >> if ( error ) > ... this still is a behavioral change: There may now be -EEXIST > bubbling up to do_microcode_update(), i.e. a hypercall that > previously succeeded under the same conditions (not applying > anything after all is not an error, particularly if that's just because > what the CPU had was already at or above the level that we try > loading). Hmm. Yeah, true, but I thought at least in some situations we could save the trouble of parsing through the fw image once again.. Maybe this can be addressed by simplifying microcode_init someday Let me go back to returning bool_t from microcode_fits() and + if ( !verify_patch_size(mc_amd->mpb_size) ) + { ... + return 0; + } + if ( mc_header->patch_id <= uci->cpu_sig.rev ) + { ... + return 0; + } Thanks, -Aravind.