[PATCH V2] x86, amd_ucode: Verify max allowed patch size before apply

[PATCH V2] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan]

Each family has a stipulated max patch_size. Use this as
additional sanity check before we apply it.

In situations where the 'patch level' of the last patch file of the
fw image that can 'fit' on the current processor is lower, i.e:

 if ( mc_header->patch_id <= uci->cpu_sig.rev )

then we can simply flush out ucode_blob or NULL-ify ucode_mod_map. This
saves bit of boot time when routines are invoked during 'microcode_init'
as we don't have to update anything.

To this end, this patch returns EEXIST when above situation occurs.

While at it, fix comment at very top to indicate we support ucode
patch loading from fam10h and higher.

Changes from V1-
 - modified code per Andrew, Jan comments
 - modified error handling around 'microcode_fits' to ensure there
   are no untoward causality issues regarding returned error value.

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/x86/microcode_amd.c |   45 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index b227173..49fbfda 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -8,7 +8,7 @@
  *  Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
  *
  *  This driver allows to upgrade microcode on AMD
- *  family 0x10 and 0x11 processors.
+ *  family 0x10 and later.
  *
  *  Licensed unter the terms of the GNU General Public
  *  License version 2. See file COPYING for details.
@@ -94,7 +94,35 @@ 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)
+{
+    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
+
+    switch (boot_cpu_data.x86)
+    {
+    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;
+    }
+
+    return (patch_size <= max_size);
+}
+
+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;
@@ -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",
@@ -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 )
-- 
1.7.9.5

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

[Jan Beulich]

>>> On 28.04.14 at 18:35, <aravind.gopalakrishnan@amd.com> 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.

> @@ -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.

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).

Jan

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

[Aravind Gopalakrishnan]

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

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

[Aravind Gopalakrishnan]

On 4/29/2014 4:33 PM, Aravind Gopalakrishnan wrote:
> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>
>>> @@ -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..
>
>

couple of ideas about implementing this:

1. something similar to mwait-idle.c:
+#ifdef DEBUG
+# define pr_debug(fmt...) printk(KERN_DEBUG fmt)
+#else
+# define pr_debug(fmt...)
+#endif

2. a custom_param: something like mce_verbosity:
(reference code)
+//arav
+int ucode_verbosity;
+static void __init ucode_set_verbosity(char *str)
+{
+    if (strcmp("verbose", str) == 0)
+        ucode_verbosity = 1;
+    else
+        printk(KERN_DEBUG "Microcode verbosity level %s not recognised"
+               "use ucode_verbosity=verbose", str);
+}
+custom_param("ucode_verbosity", ucode_set_verbosity);
+
+#define pr_debug(v, fmt...) do {       \
+        if ((v) <= ucode_verbosity) \
+            printk(fmt);       \
+        } while (0)
+

and using: (example)
+        pr_debug(1,"microcode: patch size mismatch\n");

We can probably put it in a 'microcode.h' (?) file and extend functionality
to microcode_intel as well.. (microcode_intel will only need some very 
minor edits)

I have tried both options and they work fine.
(well, second option i know works for sure on AMD :) )

Thoughts?

-Aravind.

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

[Jan Beulich]

>>> On 30.04.14 at 01:56, <aravind.gopalakrishnan@amd.com> wrote:
> On 4/29/2014 4:33 PM, Aravind Gopalakrishnan wrote:
>> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>>
>>>> @@ -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..
> 
> couple of ideas about implementing this:

Actually I'd prefer to just go the microcode_intel.c route for now, unless
there's a compelling reason for something more involved.

Jan

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

[Aravind Gopalakrishnan]

On 4/30/2014 1:07 AM, Jan Beulich wrote:
>>>> On 30.04.14 at 01:56, <aravind.gopalakrishnan@amd.com> wrote:
>> On 4/29/2014 4:33 PM, Aravind Gopalakrishnan wrote:
>>> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>>>
>>>>> @@ -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..
>> couple of ideas about implementing this:
> Actually I'd prefer to just go the microcode_intel.c route for now, unless
> there's a compelling reason for something more involved.
>
My thought was using a command line parameter to toggle whether we need 
debug messages to be printed or not
is easier still than having to change #define pr_debug and recompile..

Anyway, I shall use microcode_intel's method then..

-Aravind.

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

[Boris Ostrovsky]

On 04/29/2014 05:33 PM, Aravind Gopalakrishnan wrote:
> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>>>>> On 28.04.14 at 18:35, <aravind.gopalakrishnan@amd.com> 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?
>

No,  I suspect some sort of design constraint. But the number is 
correct, I can confirm it (for what it's worth).

I am BTW not sure I understand the reason for verifying patch size. If 
you are concerned about patch/blob corruption then IIRC HW will verify 
the patch anyway and will refuse to load it if doesn't pass HW's tests. 
And if you are thinking about going over the blob's buffer size then we 
already check it in get_ucode_from_buffer_amd().

-boris

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

[Aravind Gopalakrishnan]

On 4/29/2014 7:40 PM, Boris Ostrovsky wrote:
> On 04/29/2014 05:33 PM, Aravind Gopalakrishnan wrote:
>> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>>>>>> On 28.04.14 at 18:35, <aravind.gopalakrishnan@amd.com> 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?
>>
>
> No,  I suspect some sort of design constraint. But the number is 
> correct, I can confirm it (for what it's worth).
>
> I am BTW not sure I understand the reason for verifying patch size. If 
> you are concerned about patch/blob corruption then IIRC HW will verify 
> the patch anyway and will refuse to load it if doesn't pass HW's tests.

bkdg's mention something about HW performing some 'consistency checks'. 
A check to verify max_size could very well be one among them..
But just to be on safer side, we can have this extra sanity check in SW 
before applying the patch

-Aravind.