[PATCH 0/2] Clear AMD's microcode cache on load failure

[PATCH 0/2] Clear AMD's microcode cache on load failure

[Boris Ostrovsky]

Drop microcode cache when load operation fails to update microcode.

Also make __apply_microcode_amd() return correct error.

Boris Ostrovsky (2):
  x86/microcode/AMD: Fix __apply_microcode_amd()'s return value
  x86/microcode/AMD: Clean the cache if update did not load microcode

 arch/x86/kernel/cpu/microcode/amd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.43.5

[PATCH 1/2] x86/microcode/AMD: Fix __apply_microcode_amd()'s return value

[Boris Ostrovsky]

When verify_sha256_digest() fails, __apply_microcode_amd() should propagate
the failure by returning false (and not -1 which is promoted to true)

Fixes: 50cef76d5cb0 ("x86/microcode/AMD: Load only SHA256-checksummed patches")
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 138689b8e1d8..b61028cf5c8a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -600,7 +600,7 @@ static bool __apply_microcode_amd(struct microcode_amd *mc, u32 *cur_rev,
 	unsigned long p_addr = (unsigned long)&mc->hdr.data_code;
 
 	if (!verify_sha256_digest(mc->hdr.patch_id, *cur_rev, (const u8 *)p_addr, psize))
-		return -1;
+		return false;
 
 	native_wrmsrl(MSR_AMD64_PATCH_LOADER, p_addr);
 
-- 
2.43.5

[PATCH 2/2] x86/microcode/AMD: Clean the cache if update did not load microcode

[Boris Ostrovsky]

If microcode did not get loaded there is no reason to keep it in the cache.
Moreover, if loading failed it will not be possible to load an earlier
version of microcode since failed version will always be selected from
the cache on next reload attempt.

Since failed version is not easily avaialble at this point just clean the
whole cache. It will be rebuilt later if needed.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index b61028cf5c8a..57bd61f9c69b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1171,11 +1171,18 @@ static void microcode_fini_cpu_amd(int cpu)
 	uci->mc = NULL;
 }
 
+static void finalize_late_load_amd(int result)
+{
+	if (result)
+		cleanup();
+}
+
 static struct microcode_ops microcode_amd_ops = {
 	.request_microcode_fw	= request_microcode_amd,
 	.collect_cpu_info	= collect_cpu_info_amd,
 	.apply_microcode	= apply_microcode_amd,
 	.microcode_fini_cpu	= microcode_fini_cpu_amd,
+	.finalize_late_load	= finalize_late_load_amd,
 	.nmi_safe		= true,
 };
 
-- 
2.43.5

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Ingo Molnar]

* Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> Drop microcode cache when load operation fails to update microcode.
> 
> Also make __apply_microcode_amd() return correct error.
> 
> Boris Ostrovsky (2):
>   x86/microcode/AMD: Fix __apply_microcode_amd()'s return value
>   x86/microcode/AMD: Clean the cache if update did not load microcode
> 
>  arch/x86/kernel/cpu/microcode/amd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Should these be Cc: stable perhaps?

Thanks,

	Ingo

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[boris.ostrovsky]

On 3/27/25 5:33 PM, Ingo Molnar wrote:
> * Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>
>> Drop microcode cache when load operation fails to update microcode.
>>
>> Also make __apply_microcode_amd() return correct error.
>>
>> Boris Ostrovsky (2):
>>    x86/microcode/AMD: Fix __apply_microcode_amd()'s return value
>>    x86/microcode/AMD: Clean the cache if update did not load microcode
>>
>>   arch/x86/kernel/cpu/microcode/amd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
> Should these be Cc: stable perhaps?


Definitely the first patch. The second one is not really a fix but 
rather an improvement.

Will resend. Thanks.


-boris

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Borislav Petkov]

On Thu, Mar 27, 2025 at 05:44:32PM -0400, boris.ostrovsky@oracle.com wrote:
> Will resend.

No need to resend - I'll sort it out.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Ingo Molnar]

* boris.ostrovsky@oracle.com <boris.ostrovsky@oracle.com> wrote:

> 
> On 3/27/25 5:33 PM, Ingo Molnar wrote:
> > * Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> > 
> > > Drop microcode cache when load operation fails to update microcode.
> > > 
> > > Also make __apply_microcode_amd() return correct error.
> > > 
> > > Boris Ostrovsky (2):
> > >    x86/microcode/AMD: Fix __apply_microcode_amd()'s return value
> > >    x86/microcode/AMD: Clean the cache if update did not load microcode
> > > 
> > >   arch/x86/kernel/cpu/microcode/amd.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > Should these be Cc: stable perhaps?
> 
> 
> Definitely the first patch. The second one is not really a fix but rather an
> improvement.

Well, #2 seems to be fixing a real bug too:

  If microcode did not get loaded there is no reason to keep it in the cache.
  Moreover, if loading failed it will not be possible to load an earlier
  version of microcode since failed version will always be selected from
  the cache on next reload attempt.

this bug basically regresses the ability to load an earlier version of 
the microcode, if a newer version's loading has failed.

It would be a pretty common usecase to attempt to load the earlier 
version if the loading of a new one doesn't succeed, right?

Thanks,

	Ingo

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Borislav Petkov]

On Fri, Mar 28, 2025 at 02:36:14PM +0100, Ingo Molnar wrote:
> It would be a pretty common usecase to attempt to load the earlier 
> version if the loading of a new one doesn't succeed, right?

This is only for late loading and no one should do that anyway.

And load failure almost never happens - unless you're a cloud guy doing
special hackery.

So no need to expedite this as a fix - the majority does not care.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Mar 28, 2025 at 02:36:14PM +0100, Ingo Molnar wrote:
> > It would be a pretty common usecase to attempt to load the earlier 
> > version if the loading of a new one doesn't succeed, right?
> 
> This is only for late loading and no one should do that anyway.
> 
> And load failure almost never happens - unless you're a cloud guy doing
> special hackery.
> 
> So no need to expedite this as a fix - the majority does not care.

Well, it's a regression over previous behavior, so it is a regression 
fix for an upstream change that is only a few weeks old, and it's an 
overall quality-of-life fix for those users that are affected. There's 
no exception to the expected upstreaming of regression fixes just 
because there's few users affected.

Given how simple the fix looks, and how fresh the recent big microcode 
loading changes are to begin with, I think we should apply it to 
x86/urgent and then (maybe) it can be forwarded to -stable in a few 
weeks if there's no problems. (Or not.)

Ie. my main point is that delaying it to v6.16 is not justified IMHO.

Thanks,

	Ingo

Re: [PATCH 0/2] Clear AMD's microcode cache on load failure

[Borislav Petkov]

On Fri, Mar 28, 2025 at 02:53:06PM +0100, Ingo Molnar wrote:
> Well, it's a regression over previous behavior,

It is not a regression from a previous behavior because this has always been
this way.

> so it is a regression fix for an upstream change that is only a few weeks
> old,

The late loading is not a few weeks old.

Sounds like you're confused. Find me on IRC and we can discuss it.

Thx.

-- 
Regards/Gruss,
    Boris.

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