[PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Ashok Raj]

In general users don't have the necessary information to determine
whether a late loading of a new microcode version has removed any feature
(MSR, CPUID etc) between what is currently loaded and this new microcode.
To address this issue, Intel has added a "minimum required version" field to
a previously reserved field in the file header. Microcode updates
should only be applied if the current microcode version is equal
to, or greater than this minimum required version.

Thomas made some suggestions[1] on how meta-data in the microcode file could
provide Linux with information to decide if the new microcode is suitable
candidate for late loading. But even the "simpler" option#1 requires a lot of
metadata and corresponding kernel code to parse it.

The proposal here is an even simpler option. Simply "OS visible features"
such as CPUID and MSRs are the only two examples. The microcode must not
change these OS visible features because they cause problems after late
loading.

Pseudo code for late loading is as follows:

if header.min_required_id == 0
	This is old format microcode, block late loading
else if current_ucode_version < header.min_required_id
	Current version is too old, block late loading of this microcode.
else
	OK to proceed with late loading.

Any microcode that modifies the interface to an OS-visible feature
will set the min_version to itself. This will enforce this microcode is
not suitable for late loading unless the currently loaded revision is greater
or equal to the new microcode affecting the change.

The enforcement is not in hardware and limited to kernel loader enforcing
the requirement. It is not required for early loading of microcode to
enforce this requirement, since the new features are only
evaluated after early loading in the boot process.


Test cases covered:

1. With new kernel, attempting to load an older format microcode with the
   min_rev=0 should be blocked by kernel.

   [  210.541802] Late loading denied: microcode header does not specify a
   required min version.

2. New microcode with a non-zero min_rev in the header, but the specified
   min_rev is greater than what is currently loaded in the CPU should be
   blocked by kernel.

   245.139828] microcode: Late loading denied: Current revision 0x8f685300 is too old to update, must be at 0xaa000050 version or higher. Use early loading instead.

3. New microcode with a min_rev < currently loaded should allow loading the
   microcode

4. Build initrd with microcode that has min_rev=0, or min_rev > currently
   loaded should permit early loading microcode from initrd.

[1] https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/


Tested-by: William Xie <william.xie@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---

Needs:
	https://lore.kernel.org/lkml/20220825075445.28171-1-bp@alien8.de/

v3: https://lore.kernel.org/lkml/20220817051127.3323755-1-ashok.raj@intel.com/
v2: https://lore.kernel.org/lkml/20220816043754.3258815-1-ashok.raj@intel.com/
v1: https://lore.kernel.org/lkml/20220813223825.3164861-1-ashok.raj@intel.com/

v3->v4:
- Drop the multi-step patch, provide that as a separate series later, since
  we want make more changes to mulit-step handling.
- Boris:
  * Rename fw_refresh as late_loading, don't use print_err as indicator
    for late loading
  * Changed commit and warn messages as suggested.
---
 arch/x86/include/asm/microcode.h       |  2 +-
 arch/x86/include/asm/microcode_intel.h |  4 +++-
 arch/x86/kernel/cpu/microcode/amd.c    |  4 ++--
 arch/x86/kernel/cpu/microcode/intel.c  | 31 +++++++++++++++++++++-----
 4 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 7f7800e15ed0..9df733b35912 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -33,7 +33,7 @@ enum ucode_state {
 
 struct microcode_ops {
 	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
-						  bool refresh_fw);
+						  bool late_loading);
 
 	void (*microcode_fini_cpu) (int cpu);
 
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..16b8715e0984 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,9 @@ struct microcode_header_intel {
 	unsigned int            pf;
 	unsigned int            datasize;
 	unsigned int            totalsize;
-	unsigned int            reserved[3];
+	unsigned int            reserved1;
+	unsigned int		min_req_id;
+	unsigned int            reserved3;
 };
 
 struct microcode_intel {
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 5f38dd75cbc5..c18d3f01a452 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -891,7 +891,7 @@ load_microcode_amd(bool save, u8 family, const u8 *data, size_t size)
  * These might be larger than 2K.
  */
 static enum ucode_state request_microcode_amd(int cpu, struct device *device,
-					      bool refresh_fw)
+					      bool late_loading)
 {
 	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -900,7 +900,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	const struct firmware *fw;
 
 	/* reload ucode container only on the boot cpu */
-	if (!refresh_fw || !bsp)
+	if (!late_loading || !bsp)
 		return UCODE_OK;
 
 	if (c->x86 >= 0x15)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1fcbd671f1df..332ba19e0147 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -163,13 +163,14 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 		intel_ucode_patch = p->data;
 }
 
-static int microcode_sanity_check(void *mc, int print_err)
+static int microcode_sanity_check(void *mc, int print_err, bool late_loading)
 {
 	unsigned long total_size, data_size, ext_table_size;
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header = NULL;
 	u32 sum, orig_sum, ext_sigcount = 0, i;
 	struct extended_signature *ext_sig;
+	struct ucode_cpu_info uci;
 
 	total_size = get_totalsize(mc_header);
 	data_size = get_datasize(mc_header);
@@ -240,6 +241,24 @@ static int microcode_sanity_check(void *mc, int print_err)
 		return -EINVAL;
 	}
 
+
+	/*
+	* Enforce for late-load that min_req_id is specified in the
+	* header. Otherwise its an old format microcode, reject it
+	*/
+	if (late_loading) {
+		if (!mc_header->min_req_id) {
+		       pr_warn("Late loading denied: Microcode header does not specify a required min version\n");
+		       return -EINVAL;
+		}
+		intel_cpu_collect_info(&uci);
+		if (uci.cpu_sig.rev < mc_header->min_req_id) {
+			pr_warn("Late loading denied: Current revision 0x%x too old to update, must be at 0x%x or higher. Use early loading instead\n",
+		       uci.cpu_sig.rev, mc_header->min_req_id);
+		       return -EINVAL;
+		}
+	}
+
 	if (!ext_table_size)
 		return 0;
 
@@ -281,7 +300,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 		mc_size = get_totalsize(mc_header);
 		if (!mc_size ||
 		    mc_size > size ||
-		    microcode_sanity_check(data, 0) < 0)
+		    microcode_sanity_check(data, 0, false) < 0)
 			break;
 
 		size -= mc_size;
@@ -778,7 +797,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	return ret;
 }
 
-static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
+static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter, bool late_loading)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	unsigned int curr_mc_size = 0, new_mc_size = 0;
@@ -820,7 +839,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 		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) {
+		    microcode_sanity_check(mc, 1, late_loading) < 0) {
 			break;
 		}
 
@@ -886,7 +905,7 @@ static bool is_blacklisted(unsigned int cpu)
 }
 
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
-					     bool refresh_fw)
+					     bool late_loading)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	const struct firmware *firmware;
@@ -909,7 +928,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	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);
+	ret = generic_load_microcode(cpu, &iter, late_loading);
 
 	release_firmware(firmware);
 
-- 
2.32.0

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Dave Hansen]

On 8/29/22 11:04, Ashok Raj wrote:
> Any microcode that modifies the interface to an OS-visible feature
> will set the min_version to itself. This will enforce this microcode is
> not suitable for late loading unless the currently loaded revision is greater
> or equal to the new microcode affecting the change.

I know this hasn't quite made it into the normal Intel documentation
channels.  But, it would be nice to make sure that we have a _really_
solid description here of the architecture of min_rev which is *very*
close to what the Intel folks building microcode images agreed to.

This whole thing is useless if the architecture contract isn't ironclad
and agreed to by both sides.

One other thing on that note...  The CPU itself authenticates the
microcode.  The OS trusts that the CPU will verify the integrity of the
image.

But, this min_rev is not part of the image that the CPU verifies, right?
 If you get your microcode images out of the back of a van in the shady
part of town, nobody can promise that min_rev in the header follows the
rules.

I don't think we need to defend against that.  I'm not sure we really
even *can* defend against it.  It's probably good to note, though.

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Ashok Raj]

On Mon, Aug 29, 2022 at 11:36:12AM -0700, Dave Hansen wrote:
> On 8/29/22 11:04, Ashok Raj wrote:
> > Any microcode that modifies the interface to an OS-visible feature
> > will set the min_version to itself. This will enforce this microcode is
> > not suitable for late loading unless the currently loaded revision is greater
> > or equal to the new microcode affecting the change.
> 
> I know this hasn't quite made it into the normal Intel documentation
> channels.  But, it would be nice to make sure that we have a _really_
> solid description here of the architecture of min_rev which is *very*
> close to what the Intel folks building microcode images agreed to.
> 
> This whole thing is useless if the architecture contract isn't ironclad
> and agreed to by both sides.
> 
> One other thing on that note...  The CPU itself authenticates the
> microcode.  The OS trusts that the CPU will verify the integrity of the
> image.
> 
> But, this min_rev is not part of the image that the CPU verifies, right?
>  If you get your microcode images out of the back of a van in the shady
> part of town, nobody can promise that min_rev in the header follows the
> rules.
> 
> I don't think we need to defend against that.  I'm not sure we really
> even *can* defend against it.  It's probably good to note, though.

If someone also recomputes checksums in the main-header and also for
extended signature tables when one is present.. just a binary edit will be
caught by the microcode_sanity_check() today.

The commit log already has the following text. It doesn't say this is not
in the encrypted header, but just says HW doesn't check for min-rev. 

If you think we should change the description below, I can update it.

------
The enforcement is not in hardware and limited to kernel loader enforcing
the requirement. It is not required for early loading of microcode to
enforce this requirement, since the new features are only
evaluated after early loading in the boot process.

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Dave Hansen]

On 8/29/22 11:52, Ashok Raj wrote:
> The enforcement is not in hardware and limited to kernel loader enforcing
> the requirement. It is not required for early loading of microcode to
> enforce this requirement, since the new features are only
> evaluated after early loading in the boot process.

That's _related_ to what I was asking, but it doesn't quite cover it.

Right now, the min_rev guarantee is something along the lines of:

	Intel will always set min-rev its its microcode releases when
	software-visible features change between microcode revisions.

That's subtly different from

	The microcode header will bump min-rev when software-
	visible features change between microcode revisions.

The kernel (and its developers) should at least be *aware* that features
can change even if there's no min-rev bump.  That could be because:

	1. A user is trying to do the microcode equivalent of "modprobe
	   --force", by hacking the header
	2. The user is applying microcode from the USB stick they found
	   in the parking lot.
	3. Intel isn't sticking to its end of the bargain (or we never
	   really agreed about what the bargain was in the first place).

I'm not saying that your patch can or should do this, but the min_rev
feature does *not* mean that we can just start to forego any sanity
checks about features being added or removed as the result of a late load.

I think those three ^ cases are even worth calling out in the changelog
because it's very easy to confuse what min_rev really *MEANS* in the
end.  It only has meaning when the ucode image is unmodified between
Intel and the kernel, *AND* if Intel keeps up its end of the contract.

BTW, about #3...  I fully trust Intel to be a good actor here.  But,
Intel employs actual humans and humans do make mistakes.  Let's make
sure the kernel is resilient in the face of any mistakes.

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Borislav Petkov]

On Mon, Aug 29, 2022 at 06:04:36PM +0000, Ashok Raj wrote:
> @@ -886,7 +905,7 @@ static bool is_blacklisted(unsigned int cpu)
>  }
>  
>  static enum ucode_state request_microcode_fw(int cpu, struct device *device,
> -					     bool refresh_fw)
> +					     bool late_loading)
>  {

Until the refresh_fw's function hasn't been clarified:

https://lore.kernel.org/all/YwaBim3Xt3Il3KXm@zn.tnic/

NAK,

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Ashok Raj]

On Mon, Aug 29, 2022 at 10:31:22PM +0200, Borislav Petkov wrote:
> On Mon, Aug 29, 2022 at 06:04:36PM +0000, Ashok Raj wrote:
> > @@ -886,7 +905,7 @@ static bool is_blacklisted(unsigned int cpu)
> >  }
> >  
> >  static enum ucode_state request_microcode_fw(int cpu, struct device *device,
> > -					     bool refresh_fw)
> > +					     bool late_loading)
> >  {
> 
> Until the refresh_fw's function hasn't been clarified:
> 
> https://lore.kernel.org/all/YwaBim3Xt3Il3KXm@zn.tnic/
> 

I don't know exactly what you mean. 

But I suppose, you mean what refresh_hw is supposed to mean from the existing code?

refresh_hw seems to imply when to update  the copy of the microcode from
the filesystem. Also seems to imply late loading.

its used in the following places.

1. During reload_store() where this is exclicitly due to echo 1 > reload

	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);

	Here passing true makes sense since you are going to do a full
	refresh on all CPUs via late loading.


2. microcode_update_cpu() -> microcode_init_cpu()->request_microcode_fw(false)

   Early loading from resume. So we would use the microcode cache to load
   from.

3. mc_device_add() -> microcode_init_cpu(true)->request_microcode_fw(true)

   This seems like normal CPU hot-add, I'm not sure if refresh_fw=true is
   valid. A new CPU should also use from the cache, but not a full reload
   from filesystem. This could end up with new cpu with an updated ucode
   and older with something that was loaded earlier. Sort of what was fixed
   in:

   commit 7189b3c11903667808029ec9766a6e96de5012a5 (tag: x86_microcode_for_v5.13)


   intel.c doesn't seem to use this parameter at all today. But judging from

   amd.c: request_microcode_amd() 

        /* reload ucode container only on the boot cpu */
        if (!refresh_fw || !bsp)
                return UCODE_OK;

  Seems like it does the right job, since you would refresh only if
  refresh_fw=true and its the bsp. Hence it seems immute to the
  mc_device_add() bug.

  1. Fix mc_device_add() to set refresh_fw() to false.
  2. We can change the parameter to late_loading as you had alluded in
     previous post.
  3. Use that to distinguish if we need to enforce the
     microcode_sanity_check() to validate min_rev. 

Let me know if this is what you meant.

Cheers,
Ashok

Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

[Borislav Petkov]

On Mon, Aug 29, 2022 at 10:41:56PM +0000, Ashok Raj wrote:
> But I suppose, you mean what refresh_hw is supposed to mean from the
> existing code?

I've been meaning this for a while now.

> refresh_hw seems to imply when to update  the copy of the microcode from
> the filesystem. Also seems to imply late loading.

After your patch:

$ git grep refresh_fw arch/x86/
arch/x86/kernel/cpu/microcode/core.c:601:static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
arch/x86/kernel/cpu/microcode/core.c:616:       ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, refresh_fw);

$ git grep late_loading
arch/x86/include/asm/microcode.h:36:                                              bool late_loading);
arch/x86/kernel/cpu/microcode/amd.c:894:                                              bool late_loading)
arch/x86/kernel/cpu/microcode/amd.c:903:        if (!late_loading || !bsp)
arch/x86/kernel/cpu/microcode/intel.c:166:static int microcode_sanity_check(void *mc, int print_err, bool late_loading)
...

Now you have both. More mess.

> its used in the following places.
> 
> 1. During reload_store() where this is exclicitly due to echo 1 > reload
> 
> 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
> 
> 	Here passing true makes sense since you are going to do a full
> 	refresh on all CPUs via late loading.

Yes, and here it is perfectly clear that it is late loading.

> 2. microcode_update_cpu() -> microcode_init_cpu()->request_microcode_fw(false)
> 
>    Early loading from resume.

CPU hotplug rather.

> So we would use the microcode cache to load from.

So this happens when the CPU is coming online. I'm not sure why I set it
to "false" back then - whether it is because there's no filesystem yet
or there was another reason. I *think* this was some contrived used case
again.

In any case, this'll need to be experimented with to figure out what
happens when it is set to "true".

> 3. mc_device_add() -> microcode_init_cpu(true)->request_microcode_fw(true)
> 
>    This seems like normal CPU hot-add, I'm not sure if refresh_fw=true is
>    valid. A new CPU should also use from the cache, but not a full reload
>    from filesystem. This could end up with new cpu with an updated ucode
>    and older with something that was loaded earlier.

Just check when mc_device_add() is actually called and then try to
figure out what that actually does instead of speculating.

> Sort of what was fixed in:
>    commit 7189b3c11903667808029ec9766a6e96de5012a5 (tag: x86_microcode_for_v5.13)

That's a tag - not a git commit.

I think you mean

7189b3c11903 ("x86/microcode: Check for offline CPUs before requesting new microcode")

>    intel.c doesn't seem to use this parameter at all today. But judging from
> 
>    amd.c: request_microcode_amd() 
> 
>         /* reload ucode container only on the boot cpu */
>         if (!refresh_fw || !bsp)
>                 return UCODE_OK;
> 
>   Seems like it does the right job, since you would refresh only if
>   refresh_fw=true and its the bsp. Hence it seems immute to the
>   mc_device_add() bug.

I don't see a bug there.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette