[RFC PATCH 0/3] x86/microcode: Drop old interface and default-disable late loading

[RFC PATCH 0/3] x86/microcode: Drop old interface and default-disable late loading

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Totally untested, just sending out as a RFC first. This is something
Peter and I talked about recently and think it makes sense.

Borislav Petkov (3):
  x86/microcode: Rip out the OLD_INTERFACE
  x86/microcode: Default-disable late loading
  x86/microcode: Taint and warn on late loading

 arch/x86/Kconfig                     |  15 ++--
 arch/x86/kernel/cpu/common.c         |   2 +
 arch/x86/kernel/cpu/microcode/core.c | 111 +++------------------------
 3 files changed, 19 insertions(+), 109 deletions(-)

-- 
2.35.1

[RFC PATCH 1/3] x86/microcode: Rip out the OLD_INTERFACE

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Everything should be using the early initrd loading by now.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/Kconfig                     |  12 ----
 arch/x86/kernel/cpu/microcode/core.c | 100 ---------------------------
 2 files changed, 112 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2e8f6fd28e59..1c0da2dbfb26 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1345,18 +1345,6 @@ config MICROCODE_AMD
 	  If you select this option, microcode patch loading support for AMD
 	  processors will be enabled.
 
-config MICROCODE_OLD_INTERFACE
-	bool "Ancient loading interface (DEPRECATED)"
-	default n
-	depends on MICROCODE
-	help
-	  DO NOT USE THIS! This is the ancient /dev/cpu/microcode interface
-	  which was used by userspace tools like iucode_tool and microcode.ctl.
-	  It is inadequate because it runs too late to be able to properly
-	  load microcode on a machine and it needs special tools. Instead, you
-	  should've switched to the early loading method with the initrd or
-	  builtin microcode by now: Documentation/x86/microcode.rst
-
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
 	help
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 239ff5fcec6a..b72c4134f289 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -373,98 +373,6 @@ static int apply_microcode_on_target(int cpu)
 	return ret;
 }
 
-#ifdef CONFIG_MICROCODE_OLD_INTERFACE
-static int do_microcode_update(const void __user *buf, size_t size)
-{
-	int error = 0;
-	int cpu;
-
-	for_each_online_cpu(cpu) {
-		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-		enum ucode_state ustate;
-
-		if (!uci->valid)
-			continue;
-
-		ustate = microcode_ops->request_microcode_user(cpu, buf, size);
-		if (ustate == UCODE_ERROR) {
-			error = -1;
-			break;
-		} else if (ustate == UCODE_NEW) {
-			apply_microcode_on_target(cpu);
-		}
-	}
-
-	return error;
-}
-
-static int microcode_open(struct inode *inode, struct file *file)
-{
-	return capable(CAP_SYS_RAWIO) ? stream_open(inode, file) : -EPERM;
-}
-
-static ssize_t microcode_write(struct file *file, const char __user *buf,
-			       size_t len, loff_t *ppos)
-{
-	ssize_t ret = -EINVAL;
-	unsigned long nr_pages = totalram_pages();
-
-	if ((len >> PAGE_SHIFT) > nr_pages) {
-		pr_err("too much data (max %ld pages)\n", nr_pages);
-		return ret;
-	}
-
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-
-	if (do_microcode_update(buf, len) == 0)
-		ret = (ssize_t)len;
-
-	if (ret > 0)
-		perf_check_microcode();
-
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
-	return ret;
-}
-
-static const struct file_operations microcode_fops = {
-	.owner			= THIS_MODULE,
-	.write			= microcode_write,
-	.open			= microcode_open,
-	.llseek		= no_llseek,
-};
-
-static struct miscdevice microcode_dev = {
-	.minor			= MICROCODE_MINOR,
-	.name			= "microcode",
-	.nodename		= "cpu/microcode",
-	.fops			= &microcode_fops,
-};
-
-static int __init microcode_dev_init(void)
-{
-	int error;
-
-	error = misc_register(&microcode_dev);
-	if (error) {
-		pr_err("can't misc_register on minor=%d\n", MICROCODE_MINOR);
-		return error;
-	}
-
-	return 0;
-}
-
-static void __exit microcode_dev_exit(void)
-{
-	misc_deregister(&microcode_dev);
-}
-#else
-#define microcode_dev_init()	0
-#define microcode_dev_exit()	do { } while (0)
-#endif
-
 /* fake device for request_firmware */
 static struct platform_device	*microcode_pdev;
 
@@ -856,10 +764,6 @@ static int __init microcode_init(void)
 		goto out_driver;
 	}
 
-	error = microcode_dev_init();
-	if (error)
-		goto out_ucode_group;
-
 	register_syscore_ops(&mc_syscore_ops);
 	cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting",
 				  mc_cpu_starting, NULL);
@@ -870,10 +774,6 @@ static int __init microcode_init(void)
 
 	return 0;
 
- out_ucode_group:
-	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
-			   &cpu_root_microcode_group);
-
  out_driver:
 	cpus_read_lock();
 	mutex_lock(&microcode_mutex);
-- 
2.35.1

[RFC PATCH 2/3] x86/microcode: Default-disable late loading

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

It is dangerous and it should not be used anyway - there's a nice early
loading already.

Requested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/Kconfig                     | 11 +++++++++++
 arch/x86/kernel/cpu/common.c         |  2 ++
 arch/x86/kernel/cpu/microcode/core.c |  7 ++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 1c0da2dbfb26..33891b82fb65 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1345,6 +1345,17 @@ config MICROCODE_AMD
 	  If you select this option, microcode patch loading support for AMD
 	  processors will be enabled.
 
+config MICROCODE_LATE_LOADING
+	bool "Late microcode loading (DANGEROUS)"
+	default n
+	depends on MICROCODE
+	help
+	  Loading microcode late, when the system is up and executing instructions
+	  is a tricky business and should be avoided if possible. Just the sequence
+	  of synchronizing all cores and SMT threads is one fragile dance which does
+	  not guarantee that cores might not softlock after the loading. Therefore,
+	  use this at your own risk. Late loading taints the kernel too.
+
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
 	help
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 2e9142797c99..c296cb1c0113 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2222,6 +2222,7 @@ void cpu_init_secondary(void)
 }
 #endif
 
+#ifdef CONFIG_MICROCODE_LATE_LOADING
 /*
  * The microcode loader calls this upon late microcode load to recheck features,
  * only when microcode has been updated. Caller holds microcode_mutex and CPU
@@ -2251,6 +2252,7 @@ void microcode_check(void)
 	pr_warn("x86/CPU: CPU features have changed after loading microcode, but might not take effect.\n");
 	pr_warn("x86/CPU: Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
 }
+#endif
 
 /*
  * Invoked from core CPU hotplug code after hotplug operations
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index b72c4134f289..c717db6b6856 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -376,6 +376,7 @@ static int apply_microcode_on_target(int cpu)
 /* fake device for request_firmware */
 static struct platform_device	*microcode_pdev;
 
+#ifdef CONFIG_MICROCODE_LATE_LOADING
 /*
  * Late loading dance. Why the heavy-handed stomp_machine effort?
  *
@@ -543,6 +544,9 @@ static ssize_t reload_store(struct device *dev,
 	return ret;
 }
 
+static DEVICE_ATTR_WO(reload);
+#endif
+
 static ssize_t version_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
@@ -559,7 +563,6 @@ static ssize_t pf_show(struct device *dev,
 	return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
 }
 
-static DEVICE_ATTR_WO(reload);
 static DEVICE_ATTR(version, 0444, version_show, NULL);
 static DEVICE_ATTR(processor_flags, 0444, pf_show, NULL);
 
@@ -712,7 +715,9 @@ static int mc_cpu_down_prep(unsigned int cpu)
 }
 
 static struct attribute *cpu_root_microcode_attrs[] = {
+#ifdef CONFIG_MICROCODE_LATE_LOADING
 	&dev_attr_reload.attr,
+#endif
 	NULL
 };
 
-- 
2.35.1

Re: [RFC PATCH 2/3] x86/microcode: Default-disable late loading

[Ingo Molnar]

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

> From: Borislav Petkov <bp@suse.de>
> 
> It is dangerous and it should not be used anyway - there's a nice early
> loading already.
> 
> Requested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/Kconfig                     | 11 +++++++++++
>  arch/x86/kernel/cpu/common.c         |  2 ++
>  arch/x86/kernel/cpu/microcode/core.c |  7 ++++++-
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1c0da2dbfb26..33891b82fb65 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1345,6 +1345,17 @@ config MICROCODE_AMD
>  	  If you select this option, microcode patch loading support for AMD
>  	  processors will be enabled.
>  
> +config MICROCODE_LATE_LOADING
> +	bool "Late microcode loading (DANGEROUS)"
> +	default n
> +	depends on MICROCODE
> +	help

( Small nit: 'default n' is the default, there's no need to list it 
  explicitly - and that's the convention as well. )

> +	  Loading microcode late, when the system is up and executing instructions
> +	  is a tricky business and should be avoided if possible. Just the sequence
> +	  of synchronizing all cores and SMT threads is one fragile dance which does
> +	  not guarantee that cores might not softlock after the loading. Therefore,
> +	  use this at your own risk. Late loading taints the kernel too.

Might make sense to outline here valid circumstances under which late 
loading is used? Such as some weird kernel package that doesn't have the 
latest firmware included in the initrd?

Because it's hard (for me) to see any valid circumstance under which late 
loading should be supported at all TBH: new kernels where this patch is 
active would come with a modern package.

Ie. we should consider removing late loading altogether.

Thanks,

	Ingo

Re: [RFC PATCH 2/3] x86/microcode: Default-disable late loading

[Borislav Petkov]

On Fri, May 27, 2022 at 12:37:24PM +0200, Ingo Molnar wrote:
> Might make sense to outline here valid circumstances under which late 
> loading is used? Such as some weird kernel package that doesn't have the 
> latest firmware included in the initrd?

Well, considering how we do correctness first, functionality later, I'd
really really like to not do late loading at all.

I wasn't aware of this sequence Cooper gave me on IRC but consider what
happens where one thread is doing the microcode uploading and the other
gets an NMI!

Our "precious" dance we have right now is not protected against that
scenario so *actually*, if we have to do the right thing, we should not
do late loading at all and zap it.

> Because it's hard (for me) to see any valid circumstance under which late 
> loading should be supported at all TBH: new kernels where this patch is 
> active would come with a modern package.
> 
> Ie. we should consider removing late loading altogether.

Yap, exactly.

Late loading is actually broken as it is right now, IMNSVHO.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Warn when done and taint the kernel.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index c717db6b6856..f7ded2facaa0 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -501,6 +501,8 @@ static int microcode_reload_late(void)
 		microcode_check();
 
 	pr_info("Reload completed, microcode revision: 0x%x\n", boot_cpu_data.microcode);
+	pr_err("However, late loading is dangerous and it taints the kernel.\n"
+	       "You should switch to early loading, if possible.\n");
 
 	return ret;
 }
@@ -541,6 +543,8 @@ static ssize_t reload_store(struct device *dev,
 	if (ret == 0)
 		ret = size;
 
+	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+
 	return ret;
 }
 
-- 
2.35.1

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Luck, Tony]

On Tue, May 24, 2022 at 08:53:24PM +0200, Borislav Petkov wrote:
> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);

Seems harsh. Updating microcode to the latest is arguably the
way to make sure that your CPU stays "IN_SPEC" (since the microcode
may have a fix for a functional issue).

-Tony

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Peter Zijlstra]

On Tue, May 24, 2022 at 06:03:04PM -0700, Luck, Tony wrote:
> On Tue, May 24, 2022 at 08:53:24PM +0200, Borislav Petkov wrote:
> > +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> 
> Seems harsh. Updating microcode to the latest is arguably the
> way to make sure that your CPU stays "IN_SPEC" (since the microcode
> may have a fix for a functional issue).

Then use early loading. There's too many fails associated with late
loading.

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Borislav Petkov]

On Wed, May 25, 2022 at 08:59:40AM +0200, Peter Zijlstra wrote:
> On Tue, May 24, 2022 at 06:03:04PM -0700, Luck, Tony wrote:
> > On Tue, May 24, 2022 at 08:53:24PM +0200, Borislav Petkov wrote:
> > > +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> > 
> > Seems harsh. Updating microcode to the latest is arguably the
> > way to make sure that your CPU stays "IN_SPEC" (since the microcode
> > may have a fix for a functional issue).
> 
> Then use early loading. There's too many fails associated with late
> loading.

Yes, short of

TAINT_YOU_DID_SOMETHING_DANGEROUS

we simply don't have a better taint flag.

-- 
Regards/Gruss,
    Boris.

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

RE: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Luck, Tony]

>> Then use early loading. There's too many fails associated with late
>> loading.
>
> Yes, short of
>
> TAINT_YOU_DID_SOMETHING_DANGEROUS
>
> we simply don't have a better taint flag.

Are taint flags in such short supply that you couldn't create a new one?

The OUT_OF_SPEC one already seems to be used in some dubious
ways:
1) Command line argument to clear a X86_FEATURES bit
2) Forcing PAE
3) Writing to an MSR not on the "approved" list

As you add more ways to set this taint bit, it becomes less useful
for debugging ... since now you have to dig into which of the possible
cases set the bit to decide whether it might have contributed to the
OOPS.

-Tony

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Borislav Petkov]

On Wed, May 25, 2022 at 02:50:59PM +0000, Luck, Tony wrote:
> Are taint flags in such short supply that you couldn't create a new
> one?

Yes, they can be as many as there are letters in the english alphabet,
it seems:

struct taint_flag {
        char c_true;    /* character printed when tainted */
	^^^^^^^^^^^^

and there are already

#define TAINT_FLAGS_COUNT               18

in use.

> The OUT_OF_SPEC one already seems to be used in some dubious
> ways:
> 1) Command line argument to clear a X86_FEATURES bit
> 2) Forcing PAE
> 3) Writing to an MSR not on the "approved" list
> 
> As you add more ways to set this taint bit, it becomes less useful
> for debugging ...

Look at the other taint flags - they're set in a bunch of different
places so it is hard to unambiguously decide where the taint was set. If
we wanna use it for debugging, then the taint_flag struct above should
probably save the caller address which set the taint... or something to
that effect.

> since now you have to dig into which of the possible cases set the bit
> to decide whether it might have contributed to the OOPS.

So I'm still not convinced this should have a special taint flag.

-- 
Regards/Gruss,
    Boris.

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

RE: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Luck, Tony]

> Yes, they can be as many as there are letters in the english alphabet,
> it seems:
>
> struct taint_flag {
>        char c_true;    /* character printed when tainted */
>	^^^^^^^^^^^^
>
> and there are already
>
> #define TAINT_FLAGS_COUNT               18

That's very 1990's thinking. We have Unicode available now.
We could start using Emoji like U+1F4A9 as a taint character :-) 

> Look at the other taint flags - they're set in a bunch of different
> places so it is hard to unambiguously decide where the taint was set. If
> we wanna use it for debugging, then the taint_flag struct above should
> probably save the caller address which set the taint... or something to
> that effect.

That sounds better than a string of Emoji (also better than the current
string of upper case characters).

-Tony

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Borislav Petkov]

On Wed, May 25, 2022 at 03:40:50PM +0000, Luck, Tony wrote:
> That's very 1990's thinking. We have Unicode available now.
> We could start using Emoji like U+1F4A9 as a taint character :-)

LOL: U+1F4A9 PILE OF POO. Some fancy terminals might even show it
properly.

And sounds about the right one in this case.

> That sounds better than a string of Emoji (also better than the
> current string of upper case characters).

Yeah, we need to think about it. I would be lying if I said I had never
wondered before where a taint flag got set. Saving the address has a
problem when multiple times the same flag gets set - only the last
address will be there.

But I guess we can try it.

/me puts on todo.

-- 
Regards/Gruss,
    Boris.

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

Taint addresses

[Borislav Petkov]

I guess something like this:

...
[    2.591532] x86/mm: Checked W+X mappings: passed, no W+X pages found.
[    2.592678] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S       C        5.18.0+ #7
[    2.593079] Last taint addresses:
[    2.593079]  S:start_kernel+0x614/0x634
[    2.593079]  C:kernel_init+0x70/0x140
[    2.593079] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[    2.593079] Call Trace:
[    2.593079]  <TASK>
[    2.593079]  dump_stack_lvl+0x38/0x49
[    2.593079]  ? rest_init+0xd0/0xd0
[    2.593079]  kernel_init+0x75/0x140
[    2.593079]  ret_from_fork+0x22/0x30
[    2.593079]  </TASK>
---

I probably should put the taint addresses after the stack trace but
other than that, it gives you where the taint was added last.

diff --git a/include/linux/panic.h b/include/linux/panic.h
index f5844908a089..7e3aeddece5f 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -94,5 +94,6 @@ extern const char *print_tainted(void);
 extern void add_taint(unsigned flag, enum lockdep_ok);
 extern int test_taint(unsigned flag);
 extern unsigned long get_taint(void);
+void print_tainted_addresses(const char *log_lvl);
 
 #endif	/* _LINUX_PANIC_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index eb4dfb932c85..78b541c8da99 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -49,6 +49,7 @@ unsigned int __read_mostly sysctl_oops_all_cpu_backtrace;
 int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
 static unsigned long tainted_mask =
 	IS_ENABLED(CONFIG_GCC_PLUGIN_RANDSTRUCT) ? (1 << TAINT_RANDSTRUCT) : 0;
+static unsigned long taint_addrs[TAINT_FLAGS_COUNT];
 static int pause_on_oops;
 static int pause_on_oops_flag;
 static DEFINE_SPINLOCK(pause_on_oops_lock);
@@ -437,6 +438,23 @@ const char *print_tainted(void)
 	return buf;
 }
 
+void print_tainted_addresses(const char *log_lvl)
+{
+	int i;
+
+	if (!tainted_mask)
+		return;
+
+	printk("%sLast taint addresses:\n", log_lvl);
+
+	for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
+		const struct taint_flag *t = &taint_flags[i];
+
+			if (test_bit(i, &tainted_mask))
+				printk("%s %c:%pS\n", log_lvl, t->c_true, (void *)taint_addrs[i]);
+	}
+}
+
 int test_taint(unsigned flag)
 {
 	return test_bit(flag, &tainted_mask);
@@ -461,7 +479,11 @@ void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
 	if (lockdep_ok == LOCKDEP_NOW_UNRELIABLE && __debug_locks_off())
 		pr_warn("Disabling lock debugging due to kernel taint\n");
 
+	if (WARN_ON_ONCE(flag >= TAINT_FLAGS_COUNT))
+		return;
+
 	set_bit(flag, &tainted_mask);
+	taint_addrs[flag] = _RET_IP_;
 
 	if (tainted_mask & panic_on_taint) {
 		panic_on_taint = 0;
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 6b7f1bf6715d..595fa8757916 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -62,6 +62,8 @@ void dump_stack_print_info(const char *log_lvl)
 	       (int)strcspn(init_utsname()->version, " "),
 	       init_utsname()->version, BUILD_ID_VAL);
 
+	print_tainted_addresses(log_lvl);
+
 	if (dump_stack_arch_desc_str[0] != '\0')
 		printk("%sHardware name: %s\n",
 		       log_lvl, dump_stack_arch_desc_str);

-- 
Regards/Gruss,
    Boris.

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

Re: Taint addresses

[Luck, Tony]

On Thu, May 26, 2022 at 02:11:12PM +0200, Borislav Petkov wrote:
> I guess something like this:
> 
> ...
> [    2.591532] x86/mm: Checked W+X mappings: passed, no W+X pages found.
> [    2.592678] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S       C        5.18.0+ #7
> [    2.593079] Last taint addresses:
> [    2.593079]  S:start_kernel+0x614/0x634
> [    2.593079]  C:kernel_init+0x70/0x140

Maybe something a little more user friendly than addresses?

If there was a new macro:

#define add_taint(flag, lockdep) __add_taint(flag, lockdep, __FILE__, __LINE__)

then renmame existing add_taint() to __add_taint() and have it save the
file/line values.

Then you could print filename:line

Also: Is it more useful to store the most recent taint of each type,
or the first of each type?

-Tony

Re: Taint addresses

[Borislav Petkov]

On Thu, May 26, 2022 at 09:41:06AM -0700, Luck, Tony wrote:
> Also: Is it more useful to store the most recent taint of each type,
> or the first of each type?

Yeah, all good suggestions and valid questions. However, I'm still not
sure about this and would like to keep it in the bag until an actual use
case appears...

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH 3/3] x86/microcode: Taint and warn on late loading

[Peter Zijlstra]

On Tue, May 24, 2022 at 08:53:24PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Warn when done and taint the kernel.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kernel/cpu/microcode/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index c717db6b6856..f7ded2facaa0 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -501,6 +501,8 @@ static int microcode_reload_late(void)
>  		microcode_check();
>  
>  	pr_info("Reload completed, microcode revision: 0x%x\n", boot_cpu_data.microcode);
> +	pr_err("However, late loading is dangerous and it taints the kernel.\n"
> +	       "You should switch to early loading, if possible.\n");

Should we not warn *before* attempting the ucode update? Should the
whole thing come unstuck, you at least have some clue.

[RFC PATCH -v2] x86/microcode: Taint and warn on late loading

[Borislav Petkov]

On Wed, May 25, 2022 at 12:03:52PM +0200, Peter Zijlstra wrote:
> Should we not warn *before* attempting the ucode update? Should the
> whole thing come unstuck, you at least have some clue.

Sure.

From: Borislav Petkov <bp@suse.de>

Warn before it is attempted and taint the kernel.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index c717db6b6856..801b44ac3851 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -493,6 +493,9 @@ static int microcode_reload_late(void)
 {
 	int ret;
 
+	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
+	pr_err("You should switch to early loading, if possible.\n");
+
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
@@ -541,6 +544,8 @@ static ssize_t reload_store(struct device *dev,
 	if (ret == 0)
 		ret = size;
 
+	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+
 	return ret;
 }
 
-- 
2.35.1

-- 
Regards/Gruss,
    Boris.

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

[PATCH 4/3] x86/microcode: Remove unnecessary perf callback default-disable late loading

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

c93dc84cbe32 ("perf/x86: Add a microcode revision check for SNB-PEBS")
checks whether the microcode revision has fixed PEBS issues.

This can happen either:

1. At PEBS init time, where the early microcode has been loaded already

2. During late loading, in the microcode_check() callback.

So remove the unnecessary call in the microcode loader init routine.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 801b44ac3851..ad57e0e4d674 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -756,10 +756,7 @@ static int __init microcode_init(void)
 
 	cpus_read_lock();
 	mutex_lock(&microcode_mutex);
-
 	error = subsys_interface_register(&mc_cpu_interface);
-	if (!error)
-		perf_check_microcode();
 	mutex_unlock(&microcode_mutex);
 	cpus_read_unlock();
 
-- 
2.35.1


-- 
Regards/Gruss,
    Boris.

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