x86/microcode: More urgent fixes

From: Borislav Petkov <[email protected]>

Hi guys,

here are fixes for three more issues with the microcode loader. First one is culminating in adding an intel_get_microcode_revision() helper which does the proper steps to get the current microcode revision on an Intel CPU and should be used everywhere instead of opencoding (and forgetting) to do a CPUID(1).

The other two are fixes for 06b8534cb728 ("x86/microcode: Rework microcode loading") which slipped through my testing.

All this is for 4.10 only as it fixes fallout from stuff which went in this merge window.

Please apply,
thanks.

Borislav Petkov (3):
x86/CPU: Add native CPUID variants returning a single datum x86/microcode: Use native CPUID to tickle out microcode revision x86/microcode/intel: Add a helper which gives the microcode revision

Junichi Nomura (2):
x86/microcode/intel: Fix allocation size of struct ucode_patch x86/microcode/intel: Use correct buffer size for saving microcode data

 arch/x86/include/asm/microcode_intel.h | 15 ++++++++
 arch/x86/include/asm/processor.h       | 18 +++++++++
 arch/x86/kernel/cpu/intel.c            | 11 ++----
 arch/x86/kernel/cpu/microcode/intel.c  | 70 +++++++++-------------------------
 4 files changed, 53 insertions(+), 61 deletions(-)

2.11.0

From: Borislav Petkov <[email protected]>

... similarly to the cpuid_<reg>() variants.

---
 arch/x86/include/asm/processor.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

index eaf100508c36..1be64da0384e 100644
@@ -219,6 +219,24 @@ static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
 	    : "memory");
 }
 
+#define native_cpuid_reg(reg)					\
+static inline unsigned int native_cpuid_##reg(unsigned int op)	\
+{								\
+	unsigned int eax = op, ebx, ecx = 0, edx;		\
+								\
+	native_cpuid(&eax, &ebx, &ecx, &edx);			\
+								\
+	return reg;						\
+}
+
+/*
+ * Native CPUID functions returning a single datum.
+ */
+native_cpuid_reg(eax)
+native_cpuid_reg(ebx)
+native_cpuid_reg(ecx)
+native_cpuid_reg(edx)
+
 static inline void load_cr3(pgd_t *pgdir)
 {
 	write_cr3(__pa(pgdir));

2.11.0

Any reason to not make these interfaces (leaf, subleaf) from the start?

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Two, actually:

1. I modelled them after the cpuid_<reg>(op) versions

2. I don't think we need the subleaf variant right now.

But, when we do, we can do that when we cross that bridge and add

native_cpuid_<reg>(leaf, subleaf)

which gets called by the native_cpuid_<reg>(leaf) variants.

Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

You are introducing a new API; makes more sense to do it right from the start. The only reason not to have a subleaf for the non-native variants is that they may decay into a function call so there is an extra cost.

But at some point we will. Just consider leaf 7.

C doesn't allow function name overloading ;) (Well, except the C11 type hacks; to the best of my knowledge that doesn't in any way support argument *count* overloading.)

This means that the naming will be awkward at best.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

The "do it right" is what I'm not sure of. Do you see any use cases for the subleaf != 0 *native* variant at all?

Most if not all users of native_cpuid() in the tree set ecx to 0.

Grr, I forgot to change the function name. I didn't mean that.

I guess *if* the need arises - and again, I'm really sceptical about it
- we should simply add the respective native version of cpuid_count():

/* Some CPUID calls want 'count' to be placed in ecx */
static inline void native_cpuid_count(unsigned int op, int count,
unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx)

{
        *eax = op;
        *ecx = count;
        native_cpuid(eax, ebx, ecx, edx);
}

Because this way you have both native and non-native versions nicely comparable wrt arguments and retvals. And there won't be any confusion wrt to "oh, the native version takes different args". This was my main intent wrt the native_cpuid_<reg>() versions - to be consistent with the cpuid_<reg>() ones.

Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

From: Borislav Petkov <[email protected]>

Intel supplies the microcode revision value in MSR 0x8b (IA32_BIOS_SIGN_ID) after CPUID(1) has been executed. Execute it each time before reading that MSR.

It used to do sync_core() which did do CPUID but

c198b121b1a1 ("x86/asm: Rewrite sync_core() to use IRET-to-self")

changed the sync_core() implementation so we better make the microcode loading case explicit, as the SDM documents it.

---
 arch/x86/kernel/cpu/intel.c           |  2 +-
 arch/x86/kernel/cpu/microcode/intel.c | 26 +++-----------------------
 2 files changed, 4 insertions(+), 24 deletions(-)

index fcd484d2bb03..2d49aa949fa1 100644
@@ -83,7 +83,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 
 		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 		/* Required by the SDM */
-		sync_core();
+		native_cpuid_eax(1);
 		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
 	}
 

index b624b54912e1..f79249fab389 100644
@@ -368,26 +368,6 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 	return patch;
 }
 
-static void cpuid_1(void)
-{
-	/*
-	 * According to the Intel SDM, Volume 3, 9.11.7:
-	 *
-	 *   CPUID returns a value in a model specific register in
-	 *   addition to its usual register return values. The
-	 *   semantics of CPUID cause it to deposit an update ID value
-	 *   in the 64-bit model-specific register at address 08BH
-	 *   (IA32_BIOS_SIGN_ID). If no update is present in the
-	 *   processor, the value in the MSR remains unmodified.
-	 *
-	 * Use native_cpuid -- this code runs very early and we don't
-	 * want to mess with paravirt.
-	 */
-	unsigned int eax = 1, ebx, ecx = 0, edx;
-
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-}
-
 static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 {
 	unsigned int val[2];

@@ -413,7 +393,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

@@ -613,7 +593,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

@@ -825,7 +805,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

2.11.0

From: Borislav Petkov <[email protected]>

Since on Intel we're required to do CPUID(1) first, before reading the microcode revision MSR, let's add a special helper which does the required steps so that we don't forget to do them next time, when we want to read the microcode revision.

---
 arch/x86/include/asm/microcode_intel.h | 15 ++++++++++++
 arch/x86/kernel/cpu/intel.c            | 11 +++------
 arch/x86/kernel/cpu/microcode/intel.c  | 43 ++++++++++------------------------
 3 files changed, 31 insertions(+), 38 deletions(-)

index 195becc6f780..e793fc9a9b20 100644
@@ -52,6 +52,21 @@ struct extended_sigtable {
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
+static inline u32 intel_get_microcode_revision(void)
+{
+	u32 rev, dummy;
+
+	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
+
+	/* As documented in the SDM: Do a CPUID 1 here */
+	native_cpuid_eax(1);
+
+	/* get the current revision from MSR 0x8B */
+	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
+
+	return rev;
+}
+
 #ifdef CONFIG_MICROCODE_INTEL
 extern void __init load_ucode_intel_bsp(void);
 extern void load_ucode_intel_ap(void);

index 2d49aa949fa1..203f860d2ab3 100644
@@ -14,6 +14,7 @@
 #include <asm/bugs.h>
 #include <asm/cpu.h>
 #include <asm/intel-family.h>
+#include <asm/microcode_intel.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>

@@ -78,14 +79,8 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 		(c->x86 == 0x6 && c->x86_model >= 0x0e))
 		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 
-	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64)) {
-		unsigned lower_word;
-
-		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-		/* Required by the SDM */
-		native_cpuid_eax(1);
-		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
-	}
+	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64))
+		c->microcode = intel_get_microcode_revision();
 
 	/*
 	 * Atom erratum AAE44/AAF40/AAG38/AAH41:

2.11.0

From: Junichi Nomura <[email protected]>

We allocate struct ucode_patch here. @size is the size of microcode data and used for kmemdup() later in this function.

---
 arch/x86/kernel/cpu/microcode/intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index faec8fa68ffd..943486589757 100644
@@ -150,7 +150,7 @@ static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
 {
 	struct ucode_patch *p;
 
-	p = kzalloc(size, GFP_KERNEL);
+	p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 

2.11.0

From: Junichi Nomura <[email protected]>

In generic_load_microcode(), curr_mc_size is the size of the last allocated buffer and since we have this performance "optimization" there to vmalloc a new buffer only when the current one is bigger, curr_mc_size ends up becoming the size of the biggest buffer we've seen so far.

However, we end up saving the microcode patch which matches our CPU and its size is not curr_mc_size but the respective mc_size during the iteration while we're staring at it.

So save that mc_size into a separate variable and use it to store the previously found microcode buffer.

Without this fix, we could get oops like this:

  BUG: unable to handle kernel paging request at ffffc9000e30f000
  IP: __memcpy+0x12/0x20
  ...
  Call Trace:
  ? kmemdup+0x43/0x60
  __alloc_microcode_buf+0x44/0x70
  save_microcode_patch+0xd4/0x150
  generic_load_microcode+0x1b8/0x260
  request_microcode_user+0x15/0x20
  microcode_write+0x91/0x100
  __vfs_write+0x34/0x120
  vfs_write+0xc1/0x130
  SyS_write+0x56/0xc0
  do_syscall_64+0x6c/0x160
  entry_SYSCALL64_slow_path+0x25/0x25

---
 arch/x86/kernel/cpu/microcode/intel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 943486589757..3f329b74e040 100644
@@ -823,7 +823,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 	u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL;
 	int new_rev = uci->cpu_sig.rev;
 	unsigned int leftover = size;
-	unsigned int curr_mc_size = 0;
+	unsigned int curr_mc_size = 0, new_mc_size = 0;
 	unsigned int csig, cpf;
 
 	while (leftover) {

@@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 			vfree(new_mc);
 			new_rev = mc_header.rev;
 			new_mc  = mc;
+			new_mc_size = mc_size;
 			mc = NULL;	/* trigger new vmalloc */
 		}
 

@@ -889,7 +890,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 	 * permanent memory. So it will be loaded early when a CPU is hot added
 	 * or resumes.
 	 */
-	save_mc_for_early(new_mc, curr_mc_size);
+	save_mc_for_early(new_mc, new_mc_size);
 
 	pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
 		 cpu, new_rev, uci->cpu_sig.rev);

2.11.0