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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/04/14 20:48, Aravind Gopalakrishnan wrote:
> 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.

Good grief this looks mad.  Ignoring for a moment what the code actually
does, lets consider what it should do.

The administrator has an option of providing a microcode blob in a
multiboot module, and instructing Xen where to look.  In this case if a
valid blob is found, Xen will want to load the microcode ASAP and cache
it for the other processors.  This can be done as early as is suitable
on the BSP.

The administrator has an option of providing a microcode blob by
hypercall (either automatically as part of their kernel microcode
patching code, or perhaps from a userspace tool in dom0).  For a valid
blob, Xen will want to cache it and apply to all active processors. 
This can be done by whichever cpu processes the hypercall.

At any point, a new cpu coming online (AP boot, S3 etc) should look at
the cached microcode and try to load it.


So it would occur to me to have something such as as presmp initcall
which sets up a cpunotifier for any cpu coming online.  It checks to see
whether there are any blobs provided at boot, tries to load them and
caches the best result.  The hypercall tries to apply the blob from
userspace and if successful, caches the new microcode and instructs all
other cpus to load this new microcode.

I dont see a need for two initcalls in the slightest, nor do I see a
need for using tasklets on boot.  If the earlier initcall hasn't found
any microcode in a multiboot module then there will be nothing for the
latter one to do.  If the earlier did find microcode, then it should be
applied automatically whenever a new cpu comes up, rather than when
explicitly prodded by the later initcall.

Does this make sense?

~Andrew

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