Skip to contentSkip to conversation

x86/microcode: Some fixes

PATCH In review Borislav Petkov 3 participants 9 messages Apr 5, 2019 – Apr 10, 2019

Subsystems (2)
lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

cover x86/microcode: Some fixes

From: Borislav Petkov <[email protected]>

Hi all,

here is a nice cleanup from Jann along with a long overdue deprecation of /dev/cpu/microcode. So overdue that it was broken for more than a year and it went unnoticed. A very good sign that it can safely go to the eternal fields of dead electrons.

Thx.

Borislav Petkov (2):
x86/microcode: Fix the ancient deprecated microcode loading method x86/microcode: Deprecate MICROCODE_OLD_INTERFACE

Jann Horn (1):
x86/microcode/intel: Refactor Intel microcode blob loading

 arch/x86/Kconfig                      | 10 +++-
 arch/x86/kernel/cpu/microcode/core.c  |  3 +-
 arch/x86/kernel/cpu/microcode/intel.c | 71 ++++++++++++++-------------
 3 files changed, 47 insertions(+), 37 deletions(-)
Signature · 1 line
2.21.0
author maintainer
#1 lore raw

PATCH 1/3 x86/microcode/intel: Refactor Intel microcode blob loading

From: Jann Horn <[email protected]>

Change generic_load_microcode() to use the iov_iter API instead of a clumsy open-coded version which has to pay attention to __user data or kernel data, depending on the loading method. This allows to avoid explicit casting between user and kernel pointers.

Because the iov_iter API makes it hard to read the same location twice, as a side effect, also fix a double-read of the microcode header (which could e.g. lead to out-of-bounds reads in microcode_sanity_check()).

Not that it matters much, only root is allowed to load microcode anyway...

[ bp: Massage a bit, sort function-local variables. ]

---
 arch/x86/kernel/cpu/microcode/intel.c | 71 ++++++++++++++-------------
 1 file changed, 36 insertions(+), 35 deletions(-)
arch/x86/kernel/cpu/microcode/intel.c +36 -35
index 16936a24795c..a44bdbe7c55e 100644
@@ -31,6 +31,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
+#include <linux/uio.h>
 #include <linux/mm.h>
 
 #include <asm/microcode_intel.h>
@@ -861,32 +862,33 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	return ret;
 }
 
-static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
-				int (*get_ucode_data)(void *, const void *, size_t))
+static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	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, new_mc_size = 0;
-	unsigned int csig, cpf;
 	enum ucode_state ret = UCODE_OK;
+	int new_rev = uci->cpu_sig.rev;
+	u8 *new_mc = NULL, *mc = NULL;
+	unsigned int csig, cpf;
 
-	while (leftover) {
+	while (iov_iter_count(iter)) {
 		struct microcode_header_intel mc_header;
-		unsigned int mc_size;
+		unsigned int mc_size, data_size;
+		u8 *data;
 
-		if (leftover < sizeof(mc_header)) {
-			pr_err("error! Truncated header in microcode data file\n");
+		if (!copy_from_iter_full(&mc_header, sizeof(mc_header), iter)) {
+			pr_err("error! Truncated or inaccessible header in microcode data file\n");
 			break;
 		}
 
-		if (get_ucode_data(&mc_header, ucode_ptr, sizeof(mc_header)))
-			break;
-
 		mc_size = get_totalsize(&mc_header);
-		if (!mc_size || mc_size > leftover) {
-			pr_err("error! Bad data in microcode data file\n");
+		if (mc_size < sizeof(mc_header)) {
+			pr_err("error! Bad data in microcode data file (totalsize too small)\n");
+			break;
+		}
+		data_size = mc_size - sizeof(mc_header);
+		if (data_size > iov_iter_count(iter)) {
+			pr_err("error! Bad data in microcode data file (truncated file?)\n");
 			break;
 		}
 
@@ -899,7 +901,9 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 			curr_mc_size = mc_size;
 		}
 
-		if (get_ucode_data(mc, ucode_ptr, mc_size) ||
+		memcpy(mc, &mc_header, sizeof(mc_header));
+		data = mc + sizeof(mc_header);
+		if (!copy_from_iter_full(data, data_size, iter) ||
 		    microcode_sanity_check(mc, 1) < 0) {
 			break;
 		}
@@ -914,14 +918,11 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 			mc = NULL;	/* trigger new vmalloc */
 			ret = UCODE_NEW;
 		}
-
-		ucode_ptr += mc_size;
-		leftover  -= mc_size;
 	}
 
 	vfree(mc);
 
-	if (leftover) {
+	if (iov_iter_count(iter)) {
 		vfree(new_mc);
 		return UCODE_ERROR;
 	}
@@ -945,12 +946,6 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 	return ret;
 }
 
-static int get_ucode_fw(void *to, const void *from, size_t n)
-{
-	memcpy(to, from, n);
-	return 0;
-}
-
 static bool is_blacklisted(unsigned int cpu)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -977,10 +972,12 @@ static bool is_blacklisted(unsigned int cpu)
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 					     bool refresh_fw)
 {
-	char name[30];
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	const struct firmware *firmware;
+	struct iov_iter iter;
 	enum ucode_state ret;
+	struct kvec kvec;
+	char name[30];
 
 	if (is_blacklisted(cpu))
 		return UCODE_NFOUND;
@@ -993,26 +990,30 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 		return UCODE_NFOUND;
 	}
 
-	ret = generic_load_microcode(cpu, (void *)firmware->data,
-				     firmware->size, &get_ucode_fw);
+	kvec.iov_base = (void *)firmware->data;
+	kvec.iov_len = firmware->size;
+	iov_iter_kvec(&iter, WRITE, &kvec, 1, firmware->size);
+	ret = generic_load_microcode(cpu, &iter);
 
 	release_firmware(firmware);
 
 	return ret;
 }
 
-static int get_ucode_user(void *to, const void *from, size_t n)
-{
-	return copy_from_user(to, from, n);
-}
-
 static enum ucode_state
 request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
+	struct iov_iter iter;
+	struct iovec iov;
+
 	if (is_blacklisted(cpu))
 		return UCODE_NFOUND;
 
-	return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
+	iov.iov_base = (void __user *)buf;
+	iov.iov_len = size;
+	iov_iter_init(&iter, WRITE, &iov, 1, size);
+
+	return generic_load_microcode(cpu, &iter);
 }
 
 static struct microcode_ops microcode_intel_ops = {
Signature · 1 line
2.21.0
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] x86/microcode/intel: Refactor Intel microcode blob loading

Quoted 7 lines · Borislav Petkov

From: Jann Horn <[email protected]>

Change generic_load_microcode() to use the iov_iter API instead of a clumsy open-coded version which has to pay attention to __user data or kernel data, depending on the loading method. This allows to avoid explicit casting between user and kernel pointers.

Quoted 6 lines

Because the iov_iter API makes it hard to read the same location twice, as a side effect, also fix a double-read of the microcode header (which could e.g. lead to out-of-bounds reads in microcode_sanity_check()).

Not that it matters much, only root is allowed to load microcode anyway...

Nice!

author maintainer
#3 lore raw

PATCH 2/3 x86/microcode: Fix the ancient deprecated microcode loading method

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

The commit in Fixes: added the new define UCODE_NEW to denote that an update should happen only when newer microcode (than installed on the system) has been found.

But it missed adjusting that for the old /dev/cpu/microcode loading interface. Fix it.

Fixes 2613f36ed965 ("x86/microcode: Attempt late loading only when new microcode is present")
Cc Jann Horn <[email protected]>
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/microcode/core.c +2 -1
index 5260185cbf7b..8a4a7823451a 100644
@@ -418,8 +418,9 @@ static int do_microcode_update(const void __user *buf, size_t size)
 		if (ustate == UCODE_ERROR) {
 			error = -1;
 			break;
-		} else if (ustate == UCODE_OK)
+		} else if (ustate == UCODE_NEW) {
 			apply_microcode_on_target(cpu);
+		}
 	}
 
 	return error;
Signature · 1 line
2.21.0
Reviewed-by on v1 2/3
#4 lore raw

Re: [PATCH 2/3] x86/microcode: Fix the ancient deprecated microcode loading method

Quoted 3 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

The commit in Fixes: added the new define UCODE_NEW to denote that an

That reads odd. What's wrong with:

Commit 2613f36ed965 added ....

or

A recent commit added

mm?

Quoted 9 lines

update should happen only when newer microcode (than installed on the system) has been found.

But it missed adjusting that for the old /dev/cpu/microcode loading interface. Fix it.

Fixes 2613f36ed965 ("x86/microcode: Attempt late loading only when new microcode is present")
Cc Jann Horn <[email protected]>

Other than that:

Quoted 23 lines
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/microcode/core.c +2 -1
index 5260185cbf7b..8a4a7823451a 100644
@@ -418,8 +418,9 @@ static int do_microcode_update(const void __user *buf, size_t size)
 		if (ustate == UCODE_ERROR) {
 			error = -1;
 			break;
-		} else if (ustate == UCODE_OK)
+		} else if (ustate == UCODE_NEW) {
 			apply_microcode_on_target(cpu);
+		}
 	}
 
 	return error;
Signature · 1 line
2.21.0
#5 lore raw

[tip:x86/microcode] x86/microcode: Fix the ancient deprecated microcode loading method

In reply to Borislav Petkov
x86/microcode: Fix the ancient deprecated microcode loading method
Commit-ID:  24613a04ad1c0588c10f4b5403ca60a73d164051
Gitweb:     https://git.kernel.org/tip/24613a04ad1c0588c10f4b5403ca60a73d164051
Author:     Borislav Petkov <[email protected]>
AuthorDate: Thu, 4 Apr 2019 22:14:07 +0200
Committer:  Borislav Petkov <[email protected]>
CommitDate: Wed, 10 Apr 2019 22:41:24 +0200

x86/microcode: Fix the ancient deprecated microcode loading method

Commit

2613f36ed965 ("x86/microcode: Attempt late loading only when new microcode is present")

added the new define UCODE_NEW to denote that an update should happen only when newer microcode (than installed on the system) has been found.

But it missed adjusting that for the old /dev/cpu/microcode loading interface. Fix it.

Fixes 2613f36ed965 ("x86/microcode: Attempt late loading only when new microcode is present")
Cc Jann Horn <[email protected]>
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/microcode/core.c +2 -1
index 5260185cbf7b..8a4a7823451a 100644
@@ -418,8 +418,9 @@ static int do_microcode_update(const void __user *buf, size_t size)
 		if (ustate == UCODE_ERROR) {
 			error = -1;
 			break;
-		} else if (ustate == UCODE_OK)
+		} else if (ustate == UCODE_NEW) {
 			apply_microcode_on_target(cpu);
+		}
 	}
 
 	return error;
author maintainer
#6 lore raw

PATCH 3/3 x86/microcode: Deprecate MICROCODE_OLD_INTERFACE

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

This is the ancient loading interface which requires special tools to be used. The bigger problem with it is that it is as inadequate for proper loading of microcode as the late microcode loading interface is because it happens just as late.

iucode_tool's manpage already warns people that it is deprecated.

Deprecate it and turn it off by default along with a big warning in the Kconfig help. It will go away sometime in the future.

---
 arch/x86/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
arch/x86/Kconfig +9 -1
index 5ad92419be19..5a0a752f3ddd 100644
@@ -1330,8 +1330,16 @@ config MICROCODE_AMD
 	  processors will be enabled.
 
 config MICROCODE_OLD_INTERFACE
-	def_bool y
+	bool "Ancient loading interface (DEPRECATED)"
+	default n
 	depends on MICROCODE
+	---help---
+	  DO NOT USE THIS! This is the ancient /dev/cpu/microcode interface
+	  which was used by userspace tools like iucode_tool and microcode.ctl.
+	  It is inadequate because it runs too late to be able to properly
+	  load microcode on a machine and it needs special tools. Instead, you
+	  should've switched to the early loading method with the initrd or
+	  builtin microcode by now: Documentation/x86/microcode.txt
 
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
Signature · 1 line
2.21.0
Reviewed-by on v1 3/3
#7 lore raw

Re: [PATCH 3/3] x86/microcode: Deprecate MICROCODE_OLD_INTERFACE

Quoted 7 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

This is the ancient loading interface which requires special tools to be used. The bigger problem with it is that it is as inadequate for proper loading of microcode as the late microcode loading interface is because it happens just as late.

Quoted 6 lines

iucode_tool's manpage already warns people that it is deprecated.

Deprecate it and turn it off by default along with a big warning in the Kconfig help. It will go away sometime in the future.

#8 lore raw

[tip:x86/microcode] x86/microcode: Deprecate MICROCODE_OLD_INTERFACE

In reply to Borislav Petkov
x86/microcode: Deprecate MICROCODE_OLD_INTERFACE
Commit-ID:  c02f48e070bde326f55bd94544ca82291f7396e3
Gitweb:     https://git.kernel.org/tip/c02f48e070bde326f55bd94544ca82291f7396e3
Author:     Borislav Petkov <[email protected]>
AuthorDate: Fri, 5 Apr 2019 06:28:11 +0200
Committer:  Borislav Petkov <[email protected]>
CommitDate: Wed, 10 Apr 2019 22:43:24 +0200

x86/microcode: Deprecate MICROCODE_OLD_INTERFACE

This is the ancient loading interface which requires special tools to be used. The bigger problem with it is that it is as inadequate for proper loading of microcode as the late microcode loading interface is because it happens just as late.

iucode_tool's manpage already warns people that it is deprecated.

Deprecate it and turn it off by default along with a big fat warning in the Kconfig help. It will go away sometime in the future.

---
 arch/x86/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
arch/x86/Kconfig +9 -1
index 5ad92419be19..5a0a752f3ddd 100644
@@ -1330,8 +1330,16 @@ config MICROCODE_AMD
 	  processors will be enabled.
 
 config MICROCODE_OLD_INTERFACE
-	def_bool y
+	bool "Ancient loading interface (DEPRECATED)"
+	default n
 	depends on MICROCODE
+	---help---
+	  DO NOT USE THIS! This is the ancient /dev/cpu/microcode interface
+	  which was used by userspace tools like iucode_tool and microcode.ctl.
+	  It is inadequate because it runs too late to be able to properly
+	  load microcode on a machine and it needs special tools. Instead, you
+	  should've switched to the early loading method with the initrd or
+	  builtin microcode by now: Documentation/x86/microcode.txt
 
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"