In general users don't have the necessary information to determine
whether a late-load 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.
https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/
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-load. 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. The criteria for a microcode to
be a viable late-load candidate is that no CPUID or OS visible MSR features
are removed with respect to an earlier version of the microcode.
Pseudocode for late-load is as follows:
if header.min_required_id == 0
This is old format microcode, block late-load
else if current_ucode_version < header.min_required_id
Current version is too old, block late-load of this microcode.
else
OK to proceed with late-load.
Any microcode that removes a feature will set the min_version to itself.
This will enforce this microcode is not suitable for late-loading.
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] microcode: Header MUST specify min version for late-load
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: Current revision 0x8f685300 is too old to update,
must be at 0xaa000050 version or higher
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.
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>
---
arch/x86/include/asm/microcode_intel.h | 4 +++-
arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+), 1 deletion(-)
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/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c4b11e2fbe33..1eb202ec2302 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
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);
@@ -248,6 +249,25 @@ 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 (print_err) {
+ if (!mc_header->min_req_id) {
+ pr_warn("Header MUST specify min version for late-load\n");
+ return -EINVAL;
+ }
+
+ intel_cpu_collect_info(&uci);
+ if (uci.cpu_sig.rev < mc_header->min_req_id) {
+ pr_warn("Current revision 0x%x is too old to update,"
+ "must be at 0x%x version or higher\n",
+ uci.cpu_sig.rev, mc_header->min_req_id);
+ return -EINVAL;
+ }
+ }
+
if (!ext_table_size)
return 0;
--
2.32.0On Wed, Aug 17, 2022 at 05:11:24AM +0000, Ashok Raj wrote:
> In general users don't have the necessary information to determine
> whether a late-load 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.
>
> https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/
That goes into a Link: tag.
> 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-load. But even the "simpler" option#1 requires a lot of
In all your text:
s/late-load/late loading/g
It is called CONFIG_MICROCODE_LATE_LOADING - not CONFIG_MICROCODE_LATE_LOAD.
People are confused enough already - no need for more.
> metadata and corresponding kernel code to parse it.
>
> The proposal here is an even simpler option. The criteria for a microcode to
> be a viable late-load candidate is that no CPUID or OS visible MSR features
Simply "OS visible features" - CPUID and MSRs are only two examples. The
microcode cannot change how the OS is supposed to interact with visible
features because that causes problems.
> are removed with respect to an earlier version of the microcode.
>
> Pseudocode for late-load is as follows:
Unknown word [Pseudocode] in commit message.
Suggestions: ['Pseudo code',
> if header.min_required_id == 0
> This is old format microcode, block late-load
> else if current_ucode_version < header.min_required_id
> Current version is too old, block late-load of this microcode.
> else
> OK to proceed with late-load.
>
> Any microcode that removes a feature will set the min_version to itself.
"... that modifies the interface to a OS-visible feature..."
> This will enforce this microcode is not suitable for late-loading.
>
> 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] microcode: Header MUST specify min version for late-load
Make that more user-friendly:
"microcode: 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: Current revision 0x8f685300 is too old to update,
> must be at 0xaa000050 version or higher
"microcode: Late loading denied: Current 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.
>
>
> 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>
> ---
> arch/x86/include/asm/microcode_intel.h | 4 +++-
> arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++++++++++
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> 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/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c4b11e2fbe33..1eb202ec2302 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
You can't do this in this function:
load_ucode_intel_bsp -> __load_ucode_intel -> scan_microcode -> microcode_sanity_check
which is the early path.
So you'd have to pass down the fact that you're doing late loading from
request_microcode_fw().
Now, I'm staring at that ugly refresh_fw bool arg in that function and
I *think* I did it 10 years ago because it shouldn't try to load from
the fs when it is resuming because there might not be a fs yet... or
something to that effect.
tglx might have a better idea how to check we're in the ->starting notifier...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris
> > struct microcode_intel {
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index c4b11e2fbe33..1eb202ec2302 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
>
> You can't do this in this function:
>
> load_ucode_intel_bsp -> __load_ucode_intel -> scan_microcode -> microcode_sanity_check
>
> which is the early path.
Correct, but print_err parameter is 0 when called from scan_microcode() and 1
when called from generic_load_microcode().
We do min_rev enforcement only when print_err is set.
Should I change the parameter to "late_loading" instead so the
meaning is clear. Let me know if I that's preferred.
Cheers,
Ashok
On Tue, Aug 23, 2022 at 12:08:27AM +0000, Ashok Raj wrote:
> Correct, but print_err parameter is 0 when called from scan_microcode() and 1
> when called from generic_load_microcode().
Well, scan_microcode() gets called from save_microcode_in_initrd() which
is fs_initcall and if we had to be really precise, print_err being 0
there is wrong.
Because at fs_initcall time we can very well print error messages. But
that print_err thing is an old relic so will have to get fixed some
other day.
> We do min_rev enforcement only when print_err is set.
That's wrong - you need to do min_rev enforcement only when you're
loading microcode late. I.e., to paste from my previous mail:
"So you'd have to pass down the fact that you're doing late loading from
request_microcode_fw().
Now, I'm staring at that ugly refresh_fw bool arg in that function and
I *think* I did it 10 years ago because it shouldn't try to load from
the fs when it is resuming because there might not be a fs yet... or
something to that effect.
tglx might have a better idea how to check we're in the ->starting
notifier..."
IOW, we're going to have to do something like
->request_microcode_fw(, ... late_loading=true)
and I wanted to reuse that refresh_fw arg instead of adding another
one...
HTH.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Aug 24, 2022 at 09:52:42PM +0200, Borislav Petkov wrote: > On Tue, Aug 23, 2022 at 12:08:27AM +0000, Ashok Raj wrote: > > Correct, but print_err parameter is 0 when called from scan_microcode() and 1 > > when called from generic_load_microcode(). > > Well, scan_microcode() gets called from save_microcode_in_initrd() which > is fs_initcall and if we had to be really precise, print_err being 0 > there is wrong. > > Because at fs_initcall time we can very well print error messages. But > that print_err thing is an old relic so will have to get fixed some > other day. Well, the code hasn't changed since 2016, and possibly they migrated from another file. > > > We do min_rev enforcement only when print_err is set. > > That's wrong - you need to do min_rev enforcement only when you're > loading microcode late. I.e., to paste from my previous mail: True, if this hasn't been used for soo long, I was hoping to simply rename the variable as late_load, and repurpose it.. As you mention we do have some good opportunity to perform some cleanups here, and could address at that time. If you feel compelled to turn the print on early boot, I could flip it and send it along with my other changes? Let me know if you prefer that. And I'll pursue what you said below. I still like the microcode_sanity_check(), it sort of falls in that category. I can add another parameter passing all the way from the request_fw... come through all the other interceptors and land in the same spot. The microcode_sanity_check() was nicely isolated Intel only function and didn't need to perform surgery where it wasn't necessary :-).. Good bang for the buck :-) > > "So you'd have to pass down the fact that you're doing late loading from > request_microcode_fw(). > > Now, I'm staring at that ugly refresh_fw bool arg in that function and > I *think* I did it 10 years ago because it shouldn't try to load from > the fs when it is resuming because there might not be a fs yet... or > something to that effect. > > tglx might have a better idea how to check we're in the ->starting > notifier..." > > IOW, we're going to have to do something like > > ->request_microcode_fw(, ... late_loading=true) Sure, I'll check with how Thomas prefers it. > > and I wanted to reuse that refresh_fw arg instead of adding another > one... > > HTH. YTH! Cheers, Ashok
On Thu, Aug 25, 2022 at 04:02:07AM +0000, Ashok Raj wrote:
> If you feel compelled to turn the print on early boot, I could flip it and
> send it along with my other changes? Let me know if you prefer that.
I'd prefer that you actually read what I'm trying to explain to you.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
* Ashok Raj <ashok.raj@intel.com> wrote:
> In general users don't have the necessary information to determine
> whether a late-load 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.
>
> https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/
>
> 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-load. 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. The criteria for a microcode to
> be a viable late-load candidate is that no CPUID or OS visible MSR features
> are removed with respect to an earlier version of the microcode.
>
> Pseudocode for late-load is as follows:
>
> if header.min_required_id == 0
> This is old format microcode, block late-load
> else if current_ucode_version < header.min_required_id
> Current version is too old, block late-load of this microcode.
> else
> OK to proceed with late-load.
>
> Any microcode that removes a feature will set the min_version to itself.
> This will enforce this microcode is not suitable for late-loading.
>
> 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] microcode: Header MUST specify min version for late-load
>
> 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: Current revision 0x8f685300 is too old to update,
> must be at 0xaa000050 version or higher
>
> 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.
>
>
> 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>
> ---
> arch/x86/include/asm/microcode_intel.h | 4 +++-
> arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++++++++++
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> 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/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c4b11e2fbe33..1eb202ec2302 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
> 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);
> @@ -248,6 +249,25 @@ 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.
s/its
/it's
...
> + */
> + if (print_err) {
> + if (!mc_header->min_req_id) {
> + pr_warn("Header MUST specify min version for late-load\n");
> + return -EINVAL;
> + }
> +
> + intel_cpu_collect_info(&uci);
> + if (uci.cpu_sig.rev < mc_header->min_req_id) {
> + pr_warn("Current revision 0x%x is too old to update,"
> + "must be at 0x%x version or higher\n",
> + uci.cpu_sig.rev, mc_header->min_req_id);
Please don't line-break user-visible syslog strings, just because
checkpatch is stupid.
If the user sees it as a single line, developers should see that same line
too...
Thanks,
Ingo
© 2016 - 2026 Red Hat, Inc.