[PATCH 0/3] x86/microcode: Some fixes

[PATCH 0/3] x86/microcode: Some fixes

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

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

-- 
2.21.0

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

[Borislav Petkov]

From: Jann Horn <jannh@google.com>

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

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190404111128.131157-1-jannh@google.com
---
 arch/x86/kernel/cpu/microcode/intel.c | 71 ++++++++++++++-------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 16936a24795c..a44bdbe7c55e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -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 = {
-- 
2.21.0

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

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

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")
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Jann Horn <jannh@google.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 5260185cbf7b..8a4a7823451a 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -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;
-- 
2.21.0

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

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

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.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5ad92419be19..5a0a752f3ddd 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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"
-- 
2.21.0

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

[Thomas Gleixner]

On Fri, 5 Apr 2019, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> 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.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

[Thomas Gleixner]

On Fri, 5 Apr 2019, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> 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?

> 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")
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Jann Horn <jannh@google.com>

Other than that:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/kernel/cpu/microcode/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 5260185cbf7b..8a4a7823451a 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -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;
> -- 
> 2.21.0
> 
> 

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

[Thomas Gleixner]

On Fri, 5 Apr 2019, Borislav Petkov wrote:

> From: Jann Horn <jannh@google.com>
> 
> 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...

Nice!

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

[tip-bot for Borislav Petkov]

Commit-ID:  24613a04ad1c0588c10f4b5403ca60a73d164051
Gitweb:     https://git.kernel.org/tip/24613a04ad1c0588c10f4b5403ca60a73d164051
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Thu, 4 Apr 2019 22:14:07 +0200
Committer:  Borislav Petkov <bp@suse.de>
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")
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com>
Link: https://lkml.kernel.org/r/20190405133010.24249-3-bp@alien8.de
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 5260185cbf7b..8a4a7823451a 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -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;

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

[tip-bot for Borislav Petkov]

Commit-ID:  c02f48e070bde326f55bd94544ca82291f7396e3
Gitweb:     https://git.kernel.org/tip/c02f48e070bde326f55bd94544ca82291f7396e3
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Fri, 5 Apr 2019 06:28:11 +0200
Committer:  Borislav Petkov <bp@suse.de>
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.

Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
Link: https://lkml.kernel.org/r/20190405133010.24249-4-bp@alien8.de
---
 arch/x86/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5ad92419be19..5a0a752f3ddd 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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"