[PATCH stable] x86/microcode: do not cache microcode if it will not be used

[PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Paolo Bonzini]

[ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]

Builtin/initrd microcode will not be used the ucode loader is disabled.
But currently, save_microcode_in_initrd is always performed and it
accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
implement the MSR and the result is a call trace at boot for TDX guests.

Mainline Linux fixed this as part of a more complex rework of microcode
caching that went into 6.7 (see in particular commits dd5e3e3ca6,
"x86/microcode/intel: Simplify early loading"; and a7939f0167203,
"x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
minimum in stable kernels, setting initrd_gone just like mainline Linux
does in mark_initrd_gone().

Note that save_microcode_in_initrd() is not in the microcode application
path, which runs with paging disabled on 32-bit systems, so it can (and
has to) use dis_ucode_ldr instead of check_loader_disabled_ap().

Cc: stable@vger.kernel.org # v6.6+
Cc: x86@kernel.org # v6.6+
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 35d39a13dc90..503b5da56685 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -214,6 +214,11 @@ static int __init save_microcode_in_initrd(void)
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	int ret = -EINVAL;
 
+	if (dis_ucode_ldr) {
+		ret = 0;
+		goto out;
+	}
+
 	switch (c->x86_vendor) {
 	case X86_VENDOR_INTEL:
 		if (c->x86 >= 6)
@@ -227,6 +230,7 @@ static int __init save_microcode_in_initrd(void)
 		break;
 	}
 
+out:
 	initrd_gone = true;
 
 	return ret;
-- 
2.43.0

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Borislav Petkov]

On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> 
> Builtin/initrd microcode will not be used the ucode loader is disabled.
> But currently, save_microcode_in_initrd is always performed and it
> accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> implement the MSR and the result is a call trace at boot for TDX guests.
> 
> Mainline Linux fixed this as part of a more complex rework of microcode
> caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> minimum in stable kernels, setting initrd_gone just like mainline Linux
> does in mark_initrd_gone().
> 
> Note that save_microcode_in_initrd() is not in the microcode application
> path, which runs with paging disabled on 32-bit systems, so it can (and
> has to) use dis_ucode_ldr instead of check_loader_disabled_ap().

... simply because save_microcode_in_initrd() is a fs_initcall and by
then 32-bit has managed to enable paging.

> Cc: stable@vger.kernel.org # v6.6+
> Cc: x86@kernel.org # v6.6+
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 35d39a13dc90..503b5da56685 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -214,6 +214,11 @@ static int __init save_microcode_in_initrd(void)
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  	int ret = -EINVAL;
>  
> +	if (dis_ucode_ldr) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	switch (c->x86_vendor) {
>  	case X86_VENDOR_INTEL:
>  		if (c->x86 >= 6)
> @@ -227,6 +230,7 @@ static int __init save_microcode_in_initrd(void)
>  		break;
>  	}
>  
> +out:
>  	initrd_gone = true;
>  
>  	return ret;
> -- 

Makes sense to me.

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Greg KH]

On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]

This really isn't this commit id, sorry.

> Builtin/initrd microcode will not be used the ucode loader is disabled.
> But currently, save_microcode_in_initrd is always performed and it
> accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> implement the MSR and the result is a call trace at boot for TDX guests.
> 
> Mainline Linux fixed this as part of a more complex rework of microcode
> caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> minimum in stable kernels, setting initrd_gone just like mainline Linux
> does in mark_initrd_gone().

Why can't we take the changes in 6.7?  Doing a one-off almost always
causes problems :(

What exact commits are needed?

thanks,

greg

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Paolo Bonzini]

On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
>
> This really isn't this commit id, sorry.

True, that's the point of the mainline kernel where the logic most
closely resembles the patch. stable-kernel-rules.rst does not quite
say what to do in this case.

> > Builtin/initrd microcode will not be used the ucode loader is disabled.
> > But currently, save_microcode_in_initrd is always performed and it
> > accesses MSR_IA32_UCODE_REV even if dis_ucode_ldr is true, and in
> > particular even if X86_FEATURE_HYPERVISOR is set; the TDX module does not
> > implement the MSR and the result is a call trace at boot for TDX guests.
> >
> > Mainline Linux fixed this as part of a more complex rework of microcode
> > caching that went into 6.7 (see in particular commits dd5e3e3ca6,
> > "x86/microcode/intel: Simplify early loading"; and a7939f0167203,
> > "x86/microcode/amd: Cache builtin/initrd microcode early").  Do the bare
> > minimum in stable kernels, setting initrd_gone just like mainline Linux
> > does in mark_initrd_gone().
>
> Why can't we take the changes in 6.7?  Doing a one-off almost always
> causes problems :(

The series is
https://lore.kernel.org/all/20231002115506.217091296@linutronix.de/

+ fixes at
https://lore.kernel.org/lkml/20231010150702.495139089@linutronix.de/T/

for a total of 35 patches and 800 lines changed.

There is no individual patch that fixes the bug, because it wasn't
really intentional in those patches; it just came out naturally as a
consequence of cleaning up unnecessary code. It is fixed as of patch
7, so one possibility would be to apply patches 1-7 in the series
above but, especially for older kernels, that would be way more scary
than this three line patch. It literally says "if microcode won't be
loaded in the processor don't keep it in memory either".

Paolo

> What exact commits are needed?
>
> thanks,
>
> greg
>

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Greg KH]

On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> >
> > This really isn't this commit id, sorry.
> 
> True, that's the point of the mainline kernel where the logic most
> closely resembles the patch. stable-kernel-rules.rst does not quite
> say what to do in this case.

Ok, then just say, "this is not upstream" and the rest of your changelog
is good.  I'll edit it up tomorrow and apply it, thanks.

greg k-h

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Greg KH]

On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > >
> > > This really isn't this commit id, sorry.
> > 
> > True, that's the point of the mainline kernel where the logic most
> > closely resembles the patch. stable-kernel-rules.rst does not quite
> > say what to do in this case.
> 
> Ok, then just say, "this is not upstream" and the rest of your changelog
> is good.  I'll edit it up tomorrow and apply it, thanks.

Ok, now queued up for 6.6.y, but what about older kernel versions?  Is
this relevant for them as well?  I will note that it does seem to apply
to them properly and build, is it not relevant?

thanks,

greg k-h

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Paolo Bonzini]

On Wed, Jan 17, 2024 at 12:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> > On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > > >
> > > > This really isn't this commit id, sorry.
> > >
> > > True, that's the point of the mainline kernel where the logic most
> > > closely resembles the patch. stable-kernel-rules.rst does not quite
> > > say what to do in this case.
> >
> > Ok, then just say, "this is not upstream" and the rest of your changelog
> > is good.  I'll edit it up tomorrow and apply it, thanks.
>
> Ok, now queued up for 6.6.y, but what about older kernel versions?

6.6 is where I tested that it works, and I didn't want to put an old
kernel version in the "Cc" line, without even testing that a
non-upstream patch applies there.

The benefit would be absolutely marginal. People playing with Intel
TDX are not going to use old kernels (6.1 counts as old) anyway, for
example support for lazy acceptance of memory went into 6.5.

Paolo

Re: [PATCH stable] x86/microcode: do not cache microcode if it will not be used

[Greg KH]

On Wed, Jan 17, 2024 at 12:57:40PM +0100, Paolo Bonzini wrote:
> On Wed, Jan 17, 2024 at 12:00 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 15, 2024 at 09:59:23PM +0100, Greg KH wrote:
> > > On Mon, Jan 15, 2024 at 07:54:59PM +0100, Paolo Bonzini wrote:
> > > > On Mon, Jan 15, 2024 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Mon, Jan 15, 2024 at 11:22:02AM +0100, Paolo Bonzini wrote:
> > > > > > [ Upstream commit a7939f01672034a58ad3fdbce69bb6c665ce0024 ]
> > > > >
> > > > > This really isn't this commit id, sorry.
> > > >
> > > > True, that's the point of the mainline kernel where the logic most
> > > > closely resembles the patch. stable-kernel-rules.rst does not quite
> > > > say what to do in this case.
> > >
> > > Ok, then just say, "this is not upstream" and the rest of your changelog
> > > is good.  I'll edit it up tomorrow and apply it, thanks.
> >
> > Ok, now queued up for 6.6.y, but what about older kernel versions?
> 
> 6.6 is where I tested that it works, and I didn't want to put an old
> kernel version in the "Cc" line, without even testing that a
> non-upstream patch applies there.
> 
> The benefit would be absolutely marginal. People playing with Intel
> TDX are not going to use old kernels (6.1 counts as old) anyway, for
> example support for lazy acceptance of memory went into 6.5.

Fair enough, thanks for the explaination, I'll leave it alone then.

greg k-h