From: Borislav Petkov <[email protected]>
Currently, the patch application logic checks whether patch application
is needed. Therefore, on SMT designs where the microcode engine is
shared between the two threads, the application happens only on one of
them.
However, there are microcode patches which do per-thread modification,
see Link tag below.
Therefore, drop the revision check and try applying on each thread. This
is what the BIOS does too so this method is very much tested.
Reported-by: Ștefan Talpalaru <[email protected]>
Tested-by: Ștefan Talpalaru <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211
---
arch/x86/kernel/cpu/microcode/amd.c | 39 +++++++----------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 8b2fcdfa6d31..a575dbb4d80c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -420,8 +420,8 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
struct cont_desc desc = { 0 };
u8 (*patch)[PATCH_MAX_SIZE];
struct microcode_amd *mc;
- u32 rev, dummy, *new_rev;
bool ret = false;
+ u32 *new_rev;
#ifdef CONFIG_X86_32
new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
@@ -439,10 +439,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
if (!mc)
return ret;
- native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
- if (rev >= mc->hdr.patch_id)
- return ret;
-
if (!__apply_microcode_amd(mc)) {
*new_rev = mc->hdr.patch_id;
ret = true;
@@ -516,7 +512,7 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
{
struct microcode_amd *mc;
struct cpio_data cp;
- u32 *new_rev, rev, dummy;
+ u32 *new_rev;
if (IS_ENABLED(CONFIG_X86_32)) {
mc = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
@@ -526,10 +522,8 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
new_rev = &ucode_new_rev;
}
- native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
/* Check whether we have saved a new patch already: */
- if (*new_rev && rev < mc->hdr.patch_id) {
+ if (*new_rev) {
if (!__apply_microcode_amd(mc)) {
*new_rev = mc->hdr.patch_id;
return;
@@ -571,23 +565,17 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
void reload_ucode_amd(void)
{
- struct microcode_amd *mc;
- u32 rev, dummy __always_unused;
-
- mc = (struct microcode_amd *)amd_ucode_patch;
+ struct microcode_amd *mc = (struct microcode_amd *)amd_ucode_patch;
- rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
- if (rev < mc->hdr.patch_id) {
- if (!__apply_microcode_amd(mc)) {
- ucode_new_rev = mc->hdr.patch_id;
- pr_info("reload patch_level=0x%08x\n", ucode_new_rev);
- }
+ if (!__apply_microcode_amd(mc)) {
+ ucode_new_rev = mc->hdr.patch_id;
+ pr_info("reload patch_level=0x%08x\n", ucode_new_rev);
}
}
static u16 __find_equiv_id(unsigned int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
return find_equiv_id(&equiv_table, uci->cpu_sig.sig);
}
@@ -678,7 +666,7 @@ static enum ucode_state apply_microcode_amd(int cpu)
struct ucode_cpu_info *uci;
struct ucode_patch *p;
enum ucode_state ret;
- u32 rev, dummy __always_unused;
+ u32 rev;
BUG_ON(raw_smp_processor_id() != cpu);
@@ -691,14 +679,6 @@ static enum ucode_state apply_microcode_amd(int cpu)
mc_amd = p->data;
uci->mc = p->data;
- rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
- /* need to apply patch? */
- if (rev >= mc_amd->hdr.patch_id) {
- ret = UCODE_OK;
- goto out;
- }
-
if (__apply_microcode_amd(mc_amd)) {
pr_err("CPU%d: update failed for patch_level=0x%08x\n",
cpu, mc_amd->hdr.patch_id);
@@ -710,7 +690,6 @@ static enum ucode_state apply_microcode_amd(int cpu)
pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
-out:
uci->cpu_sig.rev = rev;
c->microcode = rev;
--
2.35.1
Hi Boris
Trying to understand if I'm missing something here.
On Sun, Aug 14, 2022 at 02:00:26PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Currently, the patch application logic checks whether patch application
> is needed. Therefore, on SMT designs where the microcode engine is
> shared between the two threads, the application happens only on one of
> them.
A re-application means, you want to apply even if the cpu_rev <= patch.rev
if cpu_rev is > patch_rev, clearly its ahead?. say BIOS has a newer version
than in the initrd image, do we want to replace the BIOS version since we do
no revid checks here.
>
> However, there are microcode patches which do per-thread modification,
> see Link tag below.
>
> Therefore, drop the revision check and try applying on each thread. This
> is what the BIOS does too so this method is very much tested.
>
> Reported-by: Ștefan Talpalaru <[email protected]>
> Tested-by: Ștefan Talpalaru <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211
> ---
> arch/x86/kernel/cpu/microcode/amd.c | 39 +++++++----------------------
> 1 file changed, 9 insertions(+), 30 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 8b2fcdfa6d31..a575dbb4d80c 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -420,8 +420,8 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
> struct cont_desc desc = { 0 };
> u8 (*patch)[PATCH_MAX_SIZE];
> struct microcode_amd *mc;
> - u32 rev, dummy, *new_rev;
> bool ret = false;
> + u32 *new_rev;
>
> #ifdef CONFIG_X86_32
> new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
> @@ -439,10 +439,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
> if (!mc)
> return ret;
>
> - native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> - if (rev >= mc->hdr.patch_id)
> - return ret;
> -
Instead of just removing the entire rev check, you want to reapply even if
the rev == patch_rev?
Worried this would allow you to go backwards as well.
if(rev > mc->hdr.patch_id)
return ret;
> if (!__apply_microcode_amd(mc)) {
> *new_rev = mc->hdr.patch_id;
> ret = true;
> @@ -516,7 +512,7 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
> {
> struct microcode_amd *mc;
> struct cpio_data cp;
> - u32 *new_rev, rev, dummy;
> + u32 *new_rev;
>
> if (IS_ENABLED(CONFIG_X86_32)) {
> mc = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> @@ -526,10 +522,8 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
> new_rev = &ucode_new_rev;
> }
>
> - native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> -
> /* Check whether we have saved a new patch already: */
> - if (*new_rev && rev < mc->hdr.patch_id) {
> + if (*new_rev) {
Here cpu_rev < mc->rev, is there a reason to remove this check?
if cpu_rev > mc->rev, the following would go backwards in rev
> if (!__apply_microcode_amd(mc)) {
> *new_rev = mc->hdr.patch_id;
> return;
> @@ -571,23 +565,17 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
>
> void reload_ucode_amd(void)
> {
> - struct microcode_amd *mc;
> - u32 rev, dummy __always_unused;
> -
> - mc = (struct microcode_amd *)amd_ucode_patch;
> + struct microcode_amd *mc = (struct microcode_amd *)amd_ucode_patch;
>
> - rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> -
> - if (rev < mc->hdr.patch_id) {
> - if (!__apply_microcode_amd(mc)) {
> - ucode_new_rev = mc->hdr.patch_id;
> - pr_info("reload patch_level=0x%08x\n", ucode_new_rev);
> - }
> + if (!__apply_microcode_amd(mc)) {
> + ucode_new_rev = mc->hdr.patch_id;
> + pr_info("reload patch_level=0x%08x\n", ucode_new_rev);
> }
> }
> static u16 __find_equiv_id(unsigned int cpu)
> {
> struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> +
> return find_equiv_id(&equiv_table, uci->cpu_sig.sig);
> }
>
> @@ -678,7 +666,7 @@ static enum ucode_state apply_microcode_amd(int cpu)
> struct ucode_cpu_info *uci;
> struct ucode_patch *p;
> enum ucode_state ret;
> - u32 rev, dummy __always_unused;
> + u32 rev;
>
> BUG_ON(raw_smp_processor_id() != cpu);
>
> @@ -691,14 +679,6 @@ static enum ucode_state apply_microcode_amd(int cpu)
> mc_amd = p->data;
> uci->mc = p->data;
>
> - rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> -
> - /* need to apply patch? */
> - if (rev >= mc_amd->hdr.patch_id) {
> - ret = UCODE_OK;
> - goto out;
> - }
> -
> if (__apply_microcode_amd(mc_amd)) {
> pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> cpu, mc_amd->hdr.patch_id);
> @@ -710,7 +690,6 @@ static enum ucode_state apply_microcode_amd(int cpu)
>
> pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
>
> -out:
> uci->cpu_sig.rev = rev;
> c->microcode = rev;
>
> --
> 2.35.1
>
On Tue, Aug 16, 2022 at 09:00:14AM +0000, Ashok Raj wrote:
> A re-application means, you want to apply even if the cpu_rev <= patch.rev
Yes.
> if cpu_rev is > patch_rev, clearly its ahead?. say BIOS has a newer
> version than in the initrd image, do we want to replace the BIOS
> version since we do no revid checks here.
Can you even downgrade the microcode through the MSR?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Aug 16, 2022 at 02:41:39PM +0200, Borislav Petkov wrote:
> On Tue, Aug 16, 2022 at 09:00:14AM +0000, Ashok Raj wrote:
> > A re-application means, you want to apply even if the cpu_rev <= patch.rev
>
> Yes.
I see, so we probably shouldn't remove the rev check completely but just
permit to reload if the rev is equal?
>
> > if cpu_rev is > patch_rev, clearly its ahead?. say BIOS has a newer
> > version than in the initrd image, do we want to replace the BIOS
> > version since we do no revid checks here.
>
> Can you even downgrade the microcode through the MSR?
I'm not sure what is true for AMD.
For Intel's there is a Security Version Number in the signed encrypted part
of the microcode. As long as the new image has SVN >= what the CPU's SVN is
you can update the microcode.
So if a rev2 is loaded, and rev1 is the new MCU, and rev2 and rev1 have the
same SVN, you can go down rev from rev2->rev1 where rev2 > rev1.
Microcode enforcement for rollback protection has been the SVN. Version
number is a SW feel good thing to identifying which image you are running,
not intended for preventing rollback or any security enforcement.
Cheers,
Ashok
Hi Boris
On Tue, Aug 16, 2022 at 02:41:39PM +0200, Borislav Petkov wrote:
> On Tue, Aug 16, 2022 at 09:00:14AM +0000, Ashok Raj wrote:
> > A re-application means, you want to apply even if the cpu_rev <= patch.rev
>
> Yes.
>
> > if cpu_rev is > patch_rev, clearly its ahead?. say BIOS has a newer
> > version than in the initrd image, do we want to replace the BIOS
> > version since we do no revid checks here.
>
> Can you even downgrade the microcode through the MSR?
>
Instead of doing a complete hack, could we maybe revive what we attempted
in 2019? At a minimum it will work for both architectures.
https://lore.kernel.org/lkml/[email protected]/
Testing microcode update is more like a unit-test and we have no luxury to
get unlimited upgraded revision numbers. But most often we might have
at least one microcode, we can play around with it, and get more results
from the community.
Back then, we just hit a wall and there was no oxygen left in the room :-)
Seems like now there is a real need for it and everyone can benefit with
something that was proposed then.
Cheers,
Ashok
On Wed, Aug 17, 2022 at 12:12:05PM +0000, Ashok Raj wrote:
> Instead of doing a complete hack,
What complete hack?!
> could we maybe revive what we attempted
> in 2019? At a minimum it will work for both architectures.
>
> https://lore.kernel.org/lkml/[email protected]/
echo 2 >... - now that's a hack.
If you wanna use the kernel for testing your hw, use a local patch. Not
expose it to people and then have me debug all kind of strange lockups.
Geez.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Aug 17, 2022 at 04:23:24PM +0200, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 12:12:05PM +0000, Ashok Raj wrote:
> > Instead of doing a complete hack,
>
> What complete hack?!
What I meant was the patch removed any and all revid checks *completely*
Instead of even limiting to == checks.
Once you do that "in spirit" its the same end goal. Isn't it? Although it
might be required for a completely different reason.
>
> > could we maybe revive what we attempted
> > in 2019? At a minimum it will work for both architectures.
> >
> > https://lore.kernel.org/lkml/[email protected]/
>
> echo 2 >... - now that's a hack.
Its all in the eye of the beholder :-)
I didn't mean in any way to bring back the echo 2. What ever you think is
suitable to enable this is fine.
>
> If you wanna use the kernel for testing your hw, use a local patch. Not
> expose it to people and then have me debug all kind of strange lockups.
So forget the hardware testing part. This is a complex flow for late-load
and how can we get more people to test it today in the community?
Do we have a more scalable way to support it today?
This is similar to XEN supporting, ucode=allow_same, there is precedence.
Doing that is just like having a built-in self-test for microcode loading.
I think there is value in enabling it for x86.
Cheers,
Ashok
On Wed, Aug 17, 2022 at 03:29:04PM +0000, Ashok Raj wrote:
> What I meant was the patch removed any and all revid checks *completely*
> Instead of even limiting to == checks.
I just tried downgrading the microcode on an AMD box. The hardware
wouldn't accept the MSR write with the lower patch ID and the higher
patch ID remained.
I'll find out whether this is universally the case on AMD.
> So forget the hardware testing part. This is a complex flow for
> late-load and how can we get more people to test it today in the
> community?
> Do we have a more scalable way to support it today?
You're not reading my mails. Lemme repeat: microcode loading is a
dangerous business, especially the late thing. I'm certainly not going
to expose that to people if there's no merit. The only merit for loading
the same revision is for testing purposes.
If you're about to test stuff, you can just as well patch the microcode
loader to do what you want it to, like I just did.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris,
On Wed, Aug 17, 2022 at 08:13:15PM +0200, Borislav Petkov wrote:
> > Do we have a more scalable way to support it today?
>
> You're not reading my mails. Lemme repeat: microcode loading is a
I do read them, but probably I'm not seeing your perspective. It's
unintentional.
> dangerous business, especially the late thing. I'm certainly not going
> to expose that to people if there's no merit. The only merit for loading
> the same revision is for testing purposes.
For this specific patch in question, its not for testing though.. Its
required for functional purposes?
>
> If you're about to test stuff, you can just as well patch the microcode
> loader to do what you want it to, like I just did.
I guess after this patch is merged, you would need no special patches out
of tree. True? I'm sorry if I missed something that is obvious.
apply_mirocode_amd() has no revid checks, and __apply_microcode_amd() has
no revid checks..
In effect you can test applying the same microcode over and over again
without having any special patches.
I thought you could enforce revid only on the primary, and siblings you
can re-apply.
Will that will satisfy the real need?
Cheers,
Ashok
On Wed, Aug 17, 2022 at 08:58:07PM +0000, Ashok Raj wrote:
> For this specific patch in question, its not for testing though.. Its
> required for functional purposes?
From its commit message:
"However, there are microcode patches which do per-thread modification,
see Link tag below.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211"
TL;DR - the application needs to happen on *every* logical CPU.
> In effect you can test applying the same microcode over and over again
> without having any special patches.
Right now, you cannot downgrade on Zen but apparently applying the same
patch over and over again works.
The patch cache still has a version check and that prevents it but
that's ugly.
I need to test how it behaves on older families first and then think
about how to do this right. Maybe use cpuinfo_x86.microcode to record
what has been applied until now...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris
On Wed, Aug 17, 2022 at 11:56:02PM +0200, Borislav Petkov wrote:
> On Wed, Aug 17, 2022 at 08:58:07PM +0000, Ashok Raj wrote:
> > For this specific patch in question, its not for testing though.. Its
> > required for functional purposes?
>
> From its commit message:
>
> "However, there are microcode patches which do per-thread modification,
> see Link tag below.
>
I did read the commit log. I was just stating the fact that reload isn't
just for testing, for e.g. in this case its required for functional
reasons.
if cpu_rev > ucode_rev, there is no need to reload microcode correct?
There are a bunch of changes in the original post that seemed like it had
nothing to do with force load on a sibling.
Completely untested, no commit log to spare you from fixing them :-)
Seemed like we were simply passing over each other with emails, thought
I'll convey what I meant here via a patch. Hope this helps.
lemme know what you think.
Cheers,
Ashok
---
arch/x86/kernel/cpu/microcode/amd.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 8b2fcdfa6d31..124e15b8559b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -440,7 +440,7 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
return ret;
native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
- if (rev >= mc->hdr.patch_id)
+ if (rev > mc->hdr.patch_id)
return ret;
if (!__apply_microcode_amd(mc)) {
@@ -679,6 +679,7 @@ static enum ucode_state apply_microcode_amd(int cpu)
struct ucode_patch *p;
enum ucode_state ret;
u32 rev, dummy __always_unused;
+ int first_cpu;
BUG_ON(raw_smp_processor_id() != cpu);
@@ -691,10 +692,17 @@ static enum ucode_state apply_microcode_amd(int cpu)
mc_amd = p->data;
uci->mc = p->data;
+ first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
/* need to apply patch? */
- if (rev >= mc_amd->hdr.patch_id) {
+ if (((cpu == first_cpu) && rev >= mc_amd->hdr.patch_id)) {
+ ret = UCODE_OK;
+ goto out;
+ }
+
+ /* Siblings need to reload microcode even if rev is same */
+ if (rev > mc_amd->hdr.patch_id) {
ret = UCODE_OK;
goto out;
}
Hi Boris
On Thu, Aug 18, 2022 at 09:58:52AM +0000, Ashok Raj wrote:
> Hi Boris
>
> On Wed, Aug 17, 2022 at 11:56:02PM +0200, Borislav Petkov wrote:
> > On Wed, Aug 17, 2022 at 08:58:07PM +0000, Ashok Raj wrote:
> > > For this specific patch in question, its not for testing though.. Its
> > > required for functional purposes?
> >
> > From its commit message:
> >
> > "However, there are microcode patches which do per-thread modification,
> > see Link tag below.
> >
>
> I did read the commit log. I was just stating the fact that reload isn't
> just for testing, for e.g. in this case its required for functional
> reasons.
>
> if cpu_rev > ucode_rev, there is no need to reload microcode correct?
> There are a bunch of changes in the original post that seemed like it had
> nothing to do with force load on a sibling.
>
> Completely untested, no commit log to spare you from fixing them :-)
>
> Seemed like we were simply passing over each other with emails, thought
> I'll convey what I meant here via a patch. Hope this helps.
>
> lemme know what you think.
I saw your original patch got tipbot notification, but I don't see the
patch in the staging area.
I didn't see a followup to this, and wasn't sure if you fixed it some other
way.
I was cleaning up my mbox and want to be sure I didn't drop the ball.
Cheers,
Ashok