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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 4/29/2014 3:02 AM, Jan Beulich wrote:
> > > > On 28.04.14 at 18:35, <aravind.gopalakrishnan@xxxxxxx> 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.


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