[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.