[5/5] x86/ucode: Drop ucode_mod and ucode_blob

Commit Message

Andrew Cooper March 27, 2023, 7:41 p.m. UTC

These both incorrectly cache bootstrap_map()'d pointers across returns back to
__start_xen().  This is never valid, and such pointers may fault, or point to
something unrelated.

With the refactoring work in the previous patches, they're clearly now just
non-standard function return parameters.

Rename struct ucode_mod_blob to just struct blob for breviy and because
there's nothing really ucode-specific about it.

Introduce find_microcode_blob(), to replace microcode_grab_module(), and
rework microcode_scan_module() so they both return a pointer/size tuple, and
don't cache their return values in global state.

This allows us to remove the microcode_init() initcall, the comments of which
gives the false impression that either of the cached pointers are safe to use.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Sergey Dyasli <[e-mail]>
---
 xen/arch/x86/cpu/microcode/core.c | 154 +++++++++++++-----------------
 1 file changed, 68 insertions(+), 86 deletions(-)

Comments

Jan Beulich March 28, 2023, 2:28 p.m. UTC | #1

On 27.03.2023 21:41, Andrew Cooper wrote:
> These both incorrectly cache bootstrap_map()'d pointers across returns back to
> __start_xen().  This is never valid, and such pointers may fault, or point to
> something unrelated.

As before - unless bootstrap_map(NULL) was (carefully) not called in
between. Same ...

> With the refactoring work in the previous patches, they're clearly now just
> non-standard function return parameters.
> 
> Rename struct ucode_mod_blob to just struct blob for breviy and because
> there's nothing really ucode-specific about it.
> 
> Introduce find_microcode_blob(), to replace microcode_grab_module(), and
> rework microcode_scan_module() so they both return a pointer/size tuple, and
> don't cache their return values in global state.
> 
> This allows us to remove the microcode_init() initcall, the comments of which
> gives the false impression that either of the cached pointers are safe to use.

... here.

> Signed-off-by: Andrew Cooper <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>
but I'd appreciate if the description was adjusted to only call the
original code as bad as it really is, not any worse.

Jan

Patch

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 7d32bc13db6f..0ae628ba99d4 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -55,7 +55,6 @@ 
  */
 #define MICROCODE_UPDATE_TIMEOUT_US 1000000
 
-static module_t __initdata ucode_mod;
 static signed int __initdata ucode_mod_idx;
 static bool_t __initdata ucode_mod_forced;
 static unsigned int nr_cores;
@@ -76,18 +75,11 @@  static enum {
     LOADING_EXIT,
 } loading_state;
 
-/*
- * If we scan the initramfs.cpio for the early microcode code
- * and find it, then 'ucode_blob' will contain the pointer
- * and the size of said blob. It is allocated from Xen's heap
- * memory.
- */
-struct ucode_mod_blob {
+struct blob {
     const void *data;
     size_t size;
 };
 
-static struct ucode_mod_blob __initdata ucode_blob;
 /*
  * By default we will NOT parse the multiboot modules to see if there is
  * cpio image with the microcode images.
@@ -148,7 +140,15 @@  static int __init cf_check parse_ucode(const char *s)
 }
 custom_param("ucode", parse_ucode);
 
-void __init microcode_scan_module(
+/*
+ * Scan all unclaimed modules, looking for a decompressed CPIO archive
+ * containing a microcode file according to the Linux early microcode API.
+ *
+ * Always caches the results, positive or negative.
+ *
+ * Returns a ptr/size tupe.  Either NULL/0, or a bootstrap_map()'d region.
+ */
+static struct blob __init microcode_scan_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
 {
@@ -159,32 +159,35 @@  void __init microcode_scan_module(
     } scan;
 
     module_t *mod = (module_t *)__va(mbi->mods_addr);
+    struct blob blob = {};
     const char *p = NULL;
     int i;
 
-    ucode_blob.size = 0;
-
     if ( scan.mod_idx ) /* Previous scan was successful. */
     {
         void *map = bootstrap_map(&mod[scan.mod_idx]);
 
         if ( !map )
-            return;
+            return blob;
+
+        blob.size = scan.size;
+        blob.data = map + scan.offset;
 
-        ucode_blob.size = scan.size;
-        ucode_blob.data = map + scan.offset;
-        return;
+        return blob;
     }
 
     if ( !ucode_scan )
-        return;
+        return blob;
+
+    /* Only scan once, whatever the outcome. */
+    ucode_scan = false;
 
     if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
         p = "kernel/x86/microcode/AuthenticAMD.bin";
     else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
         p = "kernel/x86/microcode/GenuineIntel.bin";
     else
-        return;
+        return blob;
 
     /*
      * Try all modules and see whichever could be the microcode blob.
@@ -214,30 +217,15 @@  void __init microcode_scan_module(
             scan.offset  = cd.data - _blob_start;
             scan.size    = cd.size;
 
-            ucode_blob.size = cd.size;
-            ucode_blob.data = cd.data;
+            blob.size = cd.size;
+            blob.data = cd.data;
             break;
         }
 
         bootstrap_map(NULL);
     }
-}
-
-static void __init microcode_grab_module(
-    unsigned long *module_map,
-    const multiboot_info_t *mbi)
-{
-    module_t *mod = (module_t *)__va(mbi->mods_addr);
 
-    if ( ucode_mod_idx < 0 )
-        ucode_mod_idx += mbi->mods_count;
-    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
-         !__test_and_clear_bit(ucode_mod_idx, module_map) )
-        goto scan;
-    ucode_mod = mod[ucode_mod_idx];
-scan:
-    if ( ucode_scan )
-        microcode_scan_module(module_map, mbi);
+    return blob;
 }
 
 static struct microcode_ops __ro_after_init ucode_ops;
@@ -746,28 +734,6 @@  int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
                                      microcode_update_helper, buffer);
 }
 
-static int __init cf_check microcode_init(void)
-{
-    /*
-     * At this point, all CPUs should have updated their microcode
-     * via the early_microcode_* paths so free the microcode blob.
-     */
-    if ( ucode_blob.size )
-    {
-        bootstrap_map(NULL);
-        ucode_blob.size = 0;
-        ucode_blob.data = NULL;
-    }
-    else if ( ucode_mod.mod_end )
-    {
-        bootstrap_map(NULL);
-        ucode_mod.mod_end = 0;
-    }
-
-    return 0;
-}
-__initcall(microcode_init);
-
 /* Load a cached update to current cpu */
 int microcode_update_one(void)
 {
@@ -779,26 +745,53 @@  int microcode_update_one(void)
     return microcode_update_cpu(NULL);
 }
 
+/*
+ * Find microcode within the boot modules provided.
+ *
+ * This is called twice on boot, once very early to find the BSP microcode,
+ * and a second time in order to initalise the cache once we've set up the
+ * heap to use.
+ *
+ * Returns a ptr/size tuple, either NULL/0 or a bootstrap_map()'d region.
+ */
+static struct blob __init find_microcode_blob(
+    unsigned long *module_map, const multiboot_info_t *mbi)
+{
+    struct blob blob = {};
+
+    if ( ucode_mod_idx != 0 )
+    {
+        module_t *mod = __va(mbi->mods_addr);
+
+        /* Adjust to support negative backreferences. */
+        if ( ucode_mod_idx < 0 )
+            ucode_mod_idx += mbi->mods_count;
+
+        if ( ucode_mod_idx > 0 &&
+             ucode_mod_idx < mbi->mods_count &&
+             __test_and_clear_bit(ucode_mod_idx, module_map) )
+        {
+            mod += ucode_mod_idx;
+
+            blob.data = bootstrap_map(mod);
+            blob.size = mod->mod_end;
+
+            return blob;
+        }
+
+        /* Still not valid.  Ignore it next time around. */
+        ucode_mod_idx = 0;
+    }
+
+    return microcode_scan_module(module_map, mbi);
+}
+
 int __init microcode_init_cache(unsigned long *module_map,
                                 const struct multiboot_info *mbi)
 {
     int rc = 0;
     struct microcode_patch *patch;
-    struct ucode_mod_blob blob = {};
-
-    if ( ucode_scan )
-        /* Need to rescan the modules because they might have been relocated */
-        microcode_scan_module(module_map, mbi);
-
-    if ( ucode_mod.mod_end )
-    {
-        blob.data = bootstrap_map(&ucode_mod);
-        blob.size = ucode_mod.mod_end;
-    }
-    else if ( ucode_blob.size )
-    {
-        blob = ucode_blob;
-    }
+    struct blob blob = find_microcode_blob(module_map, mbi);
 
     if ( !blob.data )
         return 0;
@@ -833,7 +826,7 @@  int __init early_microcode_init(unsigned long *module_map,
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     struct microcode_patch *patch;
-    struct ucode_mod_blob blob = {};
+    struct blob blob = {};
     int rc = 0;
 
     switch ( c->x86_vendor )
@@ -855,20 +848,9 @@  int __init early_microcode_init(unsigned long *module_map,
         return -ENODEV;
     }
 
-    microcode_grab_module(module_map, mbi);
-
     ucode_ops.collect_cpu_info();
 
-    if ( ucode_blob.data )
-    {
-        blob = ucode_blob;
-    }
-    else if ( ucode_mod.mod_end )
-    {
-        blob.data = bootstrap_map(&ucode_mod);
-        blob.size = ucode_mod.mod_end;
-    }
-
+    blob = find_microcode_blob(module_map, mbi);
     if ( !blob.data )
         return 0;