Re: [Xen-devel] [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel