2023-09-12 19:48:25

by Thomas Gleixner

[permalink] [raw]
Subject: [patch V3 30/30] x86/microcode/intel: Add a minimum required revision for late-loads

From: Ashok Raj <[email protected]>

In general users don't have the necessary information to determine whether
late loading of a new microcode version is safe and does not modify
anything which the currently running kernel uses already, e.g. removal of
CPUID bits or behavioural changes of MSRs.

To address this issue, Intel has added a "minimum required version" field
to a previously reserved field in the microcode 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 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 requires a lot of
metadata and corresponding kernel code to parse it, so the final suggestion
was to add the 'minimum required version' field in the header.

When microcode changes visible features, microcode will set the minimum
required version to its own revision which prevents late loading.

Old microcode blobs have the minimum revision field always set to 0, which
indicates that there is no information and the kernel considers it as
unsafe.

This is a pure OS software mechanism. The hardware/firmware ignores this
header field.

For early loading there is no restriction because OS visible features are
enumerated after the early load and therefor a change has no effect.

The check is always enabled, but by default not enforced. It can be
enforced via Kconfig or kernel command line.

If enforced, the kernel refuses to late load microcode with a minium
required version field which is zero or when the currently loaded microcode
revision is smaller than the minimum required revision.

If not enforced the load happens independent of the revision check to stay
compatible with the existing behaviour, but it influences the decision
whether the kernel is tainted or not. If the check signals that the late
load is safe, then the kernel is not tainted.

Early loading is not affected by this.

[ tglx: Massaged changelog and fixed up the implementation ]

Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>


---
arch/x86/include/asm/microcode.h | 3 +-
arch/x86/kernel/cpu/microcode/intel.c | 37 ++++++++++++++++++++++++++++++----
2 files changed, 35 insertions(+), 5 deletions(-)
---
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -36,7 +36,8 @@ struct microcode_header_intel {
unsigned int datasize;
unsigned int totalsize;
unsigned int metasize;
- unsigned int reserved[2];
+ unsigned int min_req_ver;
+ unsigned int reserved;
};

struct microcode_intel {
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -463,16 +463,40 @@ static enum ucode_state apply_microcode_
return ret;
}

+static bool ucode_validate_minrev(struct microcode_header_intel *mc_header)
+{
+ int cur_rev = boot_cpu_data.microcode;
+
+ /*
+ * When late-loading, ensure the header declares a minimum revision
+ * required to perform a late-load. The previously reserved field
+ * is 0 in older microcode blobs.
+ */
+ if (!mc_header->min_req_ver) {
+ pr_info("Unsafe microcode update: Microcode header does not specify a required min version\n");
+ return false;
+ }
+
+ /*
+ * Check whether the minimum revision specified in the header is either
+ * greater or equal to the current revision.
+ */
+ if (cur_rev < mc_header->min_req_ver) {
+ pr_info("Unsafe microcode update: Current revision 0x%x too old\n", cur_rev);
+ pr_info("Current should be at 0x%x or higher. Use early loading instead\n", mc_header->min_req_ver);
+ return false;
+ }
+ return true;
+}
+
static enum ucode_state read_ucode_intel(int cpu, struct iov_iter *iter)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ bool is_safe, new_is_safe = false;
int cur_rev = uci->cpu_sig.rev;
unsigned int curr_mc_size = 0;
u8 *new_mc = NULL, *mc = NULL;

- if (force_minrev)
- return UCODE_NFOUND;
-
while (iov_iter_count(iter)) {
struct microcode_header_intel mc_header;
unsigned int mc_size, data_size;
@@ -515,9 +539,14 @@ static enum ucode_state read_ucode_intel
if (!intel_find_matching_signature(mc, &uci->cpu_sig))
continue;

+ is_safe = ucode_validate_minrev(&mc_header);
+ if (force_minrev && !is_safe)
+ continue;
+
kvfree(new_mc);
cur_rev = mc_header.rev;
new_mc = mc;
+ new_is_safe = is_safe;
mc = NULL;
}

@@ -529,7 +558,7 @@ static enum ucode_state read_ucode_intel
return UCODE_NFOUND;

ucode_patch_late = (struct microcode_intel *)new_mc;
- return UCODE_NEW;
+ return new_is_safe ? UCODE_NEW_SAFE : UCODE_NEW;

fail:
kvfree(mc);


2023-09-25 23:15:41

by Qiuxu Zhuo

[permalink] [raw]
Subject: Re: [patch V3 30/30] x86/microcode/intel: Add a minimum required revision for late-loads

> ...
> From: Ashok Raj <[email protected]>
>
> In general users don't have the necessary information to determine whether
> late loading of a new microcode version is safe and does not modify
> anything which the currently running kernel uses already, e.g. removal of
> CPUID bits or behavioural changes of MSRs.
> ...
>
> The check is always enabled, but by default not enforced. It can be
> enforced via Kconfig or kernel command line.
>
> If enforced, the kernel refuses to late load microcode with a minium

s/minium/minimum/

> required version field which is zero or when the currently loaded microcode
> revision is smaller than the minimum required revision.
>
> ...
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -463,16 +463,40 @@ static enum ucode_state apply_microcode_
> return ret;
> }
>
> +static bool ucode_validate_minrev(struct microcode_header_intel *mc_header)
> +{
> + int cur_rev = boot_cpu_data.microcode;
> +
> + /*
> + * When late-loading, ensure the header declares a minimum revision
> + * required to perform a late-load. The previously reserved field
> + * is 0 in older microcode blobs.
> + */
> + if (!mc_header->min_req_ver) {
> + pr_info("Unsafe microcode update: Microcode header does not specify a required min version\n");
> + return false;
> + }
> +
> + /*
> + * Check whether the minimum revision specified in the header is either
> + * greater or equal to the current revision.
> + */

Seems like the above comment doesn't match the following 'if' check.
Perhaps the comment is:

"Check whether the current revision is either greater or
equal to the minimum revision specified in the header."

> + if (cur_rev < mc_header->min_req_ver) {
> + pr_info("Unsafe microcode update: Current revision 0x%x too old\n", cur_rev);
> + pr_info("Current should be at 0x%x or higher. Use early loading instead\n", mc_header->min_req_ver);
> + return false;
> + }
> + return true;
> +}
> ...