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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 4/25/2014 2:00 AM, Jan Beulich wrote:
> > > > On 24.04.14 at 21:54, <aravind.gopalakrishnan@xxxxxxx> 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 <Christoph.Egger@xxxxxxx>
AuthorDate: Thu Dec 15 11:00:09 2011 +0100
Commit:     Christoph Egger <Christoph.Egger@xxxxxxx>
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 <Christoph.Egger@xxxxxxx>
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Committed-by: Jan Beulich <jbeulich@xxxxxxxx>

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.

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