[Patch v3 Part2 0/9] x86/microcode: Declare microcode safe for late loading

[Patch v3 Part2 0/9] x86/microcode: Declare microcode safe for late loading

[Ashok Raj]

Hi Boris,

Here is v3 of part2. v1 Part2 is here[1]
These address feedback from Thomas here [2] posted as "Part2 v2[cleanup]"

Thanks Thomas for the feedback, and Tony for fixes to my commit logs adding
more clarity!

This series  should apply on top of tip/x86/microcode.

Please help with review and apply.

Patch 1-4:
	Cleanup patches that were in response to comments from
	Thomas[2].

Patch 5: Preparatory patch to keep warning and taint in same function.
Patch 6: Add minimum revision ID for Intel microcode
Patch 7: Add a generic mechanism to declare safe late loading.
Patch 8: Drop the unneeded wbinvd() after the minimum rev update
Patch 9: Optional - Allows testing with override minrev.

Tests Done:

1. For values other than 1 to reload file, will not report an error.
   For e.g. 

   echo 2 > reload
   bash: echo: write error: Invalid argument

2. In cases where there is no file OR no new update found, echo 1 will not
   report an error

   echo 1 > reload

3. When trying to load a file with minrev=0, there will be a log in dmesg
   and will also return -EINVAL in response to the write to "reload".

[105682.501898] microcode: Late loading denied: Microcode header does not specify a required min version

root@araj-ucode:/sys/devices/system/cpu/microcode# cat /proc/sys/kernel/tainted
0

4. Offline a primary CPU and attempt to load, must not taint the kernel.

# echo 0 > cpu2/online

[  378.725868] smpboot: CPU 2 is now offline

#echo 1 > /sys/devices/system/cpu/microcode/reload

[  388.024968] microcode: Not all CPUs online, aborting microcode update.

#cat /proc/sys/kernel/tainted
0
# echo 1 cpu2/online

[  398.664452] smpboot: Booting Node 0 Processor 2 APIC 0x4

5. When using the optional patch 9 to override minrev, kernel will be
   tainted.

root@araj-ucode:/sys/devices/system/cpu/microcode# cat /proc/sys/kernel/tainted
0
   echo Y > /sys/kernel/debug/microcode/override_minrev
   echo 1 > /sys/devices/system/cpu/microcode/reload

[  491.489986] microcode: Bypassing minrev enforcement via debugfs
[  491.649550] microcode: updated to revision 0x2b000070, date = 2022-08-22
[  493.595267] microcode: Reload succeeded, microcode revision: 0x2b000041 -> 0x2b000070
[  493.595360] microcode: Microcode late loading tainted the kernel

root@araj-ucode:/sys/devices/system/cpu/microcode# cat /proc/sys/kernel/tainted
4

[1] https://lore.kernel.org/lkml/20230113172920.113612-1-ashok.raj@intel.com/
[2] https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/

Cheers,
Ashok

Ashok Raj (9):
  x86/microcode: Taint kernel only if microcode loading was successful
  x86/microcode: Report invalid writes to reload sysfs file
  x86/microcode/intel: Fix collect_cpu_info() to reflect current
    microcode
  x86/microcode: Do not call apply_microcode() on sibling threads
  x86/microcode: Move late load warning to the same function that taints
    kernel
  x86/microcode/intel: Add minimum required revision to microcode header
  x86/microcode: Add a generic mechanism to declare support for minrev
  x86/microcode/intel: Drop wbinvd() from microcode loading
  x86/microcode: Provide an option to override minrev enforcement

 arch/x86/include/asm/microcode.h       |  4 ++
 arch/x86/include/asm/microcode_intel.h |  3 +-
 arch/x86/kernel/cpu/microcode/core.c   | 71 +++++++++++++++++++-------
 arch/x86/kernel/cpu/microcode/intel.c  | 71 +++++++++++++++++++++-----
 arch/x86/Kconfig                       |  7 +--
 5 files changed, 119 insertions(+), 37 deletions(-)


base-commit: a9a5cac225b0830d1879640e25231a37e537f0da
-- 
2.37.2

Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>

[Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

Currently when late loading is aborted due to check_online_cpu(), kernel
still ends up tainting the kernel.

Taint only when microcode loading was successful.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
v1->v2: (Thomas)
	- Remove unnecessary assignment of ret that's being overwritten.
	- Taint kernel only of loading was successful
---
 arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 61d57d9b93ee..1c6831b8b244 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
 	enum ucode_state tmp_ret = UCODE_OK;
 	int bsp = boot_cpu_data.cpu_index;
 	unsigned long val;
-	ssize_t ret = 0;
+	int load_ret = -1;
+	ssize_t ret;
 
 	ret = kstrtoul(buf, 0, &val);
 	if (ret)
@@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
 		goto put;
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
-	if (tmp_ret != UCODE_NEW)
+	if (tmp_ret != UCODE_NEW) {
+		ret = size;
 		goto put;
+	}
 
 	mutex_lock(&microcode_mutex);
-	ret = microcode_reload_late();
+	load_ret = microcode_reload_late();
 	mutex_unlock(&microcode_mutex);
 
 put:
 	cpus_read_unlock();
 
-	if (ret == 0)
+	/*
+	 * Taint only when loading was successful
+	 */
+	if (load_ret == 0) {
 		ret = size;
-
-	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		pr_warn("Microcode late loading tainted the kernel\n");
+	}
 
 	return ret;
 }
-- 
2.37.2

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Borislav Petkov]

On Mon, Jan 30, 2023 at 01:39:47PM -0800, Ashok Raj wrote:
>  arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

Why all this hoopla and unrelated changes?

Why don't you simply hoist the call to ->request_microcode_fw outside of
the locked region as it doesn't have to be there and then do the usual
pattern?

---
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 14a2280fdcd2..23f4f22df581 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -481,28 +481,28 @@ static ssize_t reload_store(struct device *dev,
 	if (val != 1)
 		return size;
 
+	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
+	if (tmp_ret != UCODE_NEW)
+		return ret;
+
 	cpus_read_lock();
 
 	ret = check_online_cpus();
 	if (ret)
-		goto put;
-
-	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
-	if (tmp_ret != UCODE_NEW)
-		goto put;
+		goto unlock;
 
 	mutex_lock(&microcode_mutex);
 	ret = microcode_reload_late();
 	mutex_unlock(&microcode_mutex);
 
-put:
-	cpus_read_unlock();
-
 	if (ret == 0)
 		ret = size;
 
 	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 
+unlock:
+	cpus_read_unlock();
+
 	return ret;
 }
 

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

On Tue, Jan 31, 2023 at 12:50:44PM +0100, Borislav Petkov wrote:
> On Mon, Jan 30, 2023 at 01:39:47PM -0800, Ashok Raj wrote:
> >  arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> Why all this hoopla and unrelated changes?
> 
> Why don't you simply hoist the call to ->request_microcode_fw outside of
> the locked region as it doesn't have to be there and then do the usual
> pattern?

Makes total sense, and seems to make the code more readable. Thanks!

Just some minor changes below.

remove ret = 0 during initialization since its cleared right below. (tglx)

Some more below, updated patch at the end.

I have tested with the modified patch below.

> 
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 14a2280fdcd2..23f4f22df581 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -481,28 +481,28 @@ static ssize_t reload_store(struct device *dev,
>  	if (val != 1)
>  		return size;
>  
> +	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> +	if (tmp_ret != UCODE_NEW)
> +		return ret;
> +
>  	cpus_read_lock();
>  
>  	ret = check_online_cpus();
>  	if (ret)
> -		goto put;
> -
> -	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> -	if (tmp_ret != UCODE_NEW)
> -		goto put;
> +		goto unlock;

Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
endlessly waiting for write to complete. (As Aubrey pointed out)

>  
>  	mutex_lock(&microcode_mutex);
>  	ret = microcode_reload_late();

I think its safe to leave ret as is, since microcode_reload_late() only
returns -1, or 0.

>  	mutex_unlock(&microcode_mutex);
>  
> -put:
> -	cpus_read_unlock();
> -
>  	if (ret == 0)
>  		ret = size;
>  
>  	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);

Pull this into the ret == 0, so taint only if the update was successful? 
And add a message so its not silent?

>  
> +unlock:
> +	cpus_read_unlock();
> +
>  	return ret;
>  }
>  
> 

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 94d942c1bf2c..550b7c566311 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -472,7 +472,7 @@ static ssize_t reload_store(struct device *dev,
 	enum ucode_state tmp_ret = UCODE_OK;
 	int bsp = boot_cpu_data.cpu_index;
 	unsigned long val;
-	ssize_t ret = 0;
+	ssize_t ret;
 
 	ret = kstrtoul(buf, 0, &val);
 	if (ret)
@@ -483,7 +483,7 @@ static ssize_t reload_store(struct device *dev,
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
 	if (tmp_ret != UCODE_NEW)
-		return ret;
+		return (tmp_ret == UCODE_ERROR ? -EINVAL : size);
 
 	cpus_read_lock();
 
@@ -495,10 +495,11 @@ static ssize_t reload_store(struct device *dev,
 	ret = microcode_reload_late();
 	mutex_unlock(&microcode_mutex);
 
-	if (ret == 0)
+	if (ret == 0) {
 		ret = size;
-
-	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		pr_warn("Microcode late loading tainted the kernel\n");
+	}
 
 unlock:
 	cpus_read_unlock();

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Borislav Petkov]

On Tue, Jan 31, 2023 at 08:51:25AM -0800, Ashok Raj wrote:
> remove ret = 0 during initialization since its cleared right below. (tglx)

Sure.

> Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
> endlessly waiting for write to complete. (As Aubrey pointed out)

Then do:

        tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
        if (tmp_ret != UCODE_NEW)
                return size;

to signal what it is. It certainly ain't an error if it doesn't find new
microcode.

> I think its safe to leave ret as is, since microcode_reload_late() only
> returns -1, or 0.

No it doesn't. Hint: stop_machine_cpuslocked().

> Pull this into the ret == 0, so taint only if the update was successful? 

Ok.

> And add a message so its not silent?

You'd add a printk for every possible operation, wouldn't you?

See, the world doesn't revolve around microcode loading. If that thing
fails, then someone has done a bad job at the CPU vendor testing,
provided the code does the right thing.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

On Tue, Jan 31, 2023 at 09:20:37PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 08:51:25AM -0800, Ashok Raj wrote:
> > remove ret = 0 during initialization since its cleared right below. (tglx)
> 
> Sure.
> 
> > Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
> > endlessly waiting for write to complete. (As Aubrey pointed out)
> 
> Then do:
> 
>         tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
>         if (tmp_ret != UCODE_NEW)
>                 return size;
> 
> to signal what it is. It certainly ain't an error if it doesn't find new
> microcode.

It's not an error, only when request_microcode() returns UCODE_ERROR, should
it return -EINVAL, if its UCODE_NFOUND, or otherwise the code should treat
as success. 

The diff I attached was: https://lore.kernel.org/lkml/Y9lHDWjjnqdletL3@a4bf019067fa.jf.intel.com/

 	if (tmp_ret != UCODE_NEW)
-		return ret;
+		return (tmp_ret == UCODE_ERROR ? -EINVAL : size);

Does the above look fine? 

> 
> > I think its safe to leave ret as is, since microcode_reload_late() only
> > returns -1, or 0.
> 
> No it doesn't. Hint: stop_machine_cpuslocked().
> 
> > Pull this into the ret == 0, so taint only if the update was successful? 
> 
> Ok.
> 
> > And add a message so its not silent?
> 
> You'd add a printk for every possible operation, wouldn't you?

:-) Not like that.. But looking through most of the cases that does
add_taint() either have some print, or there some big spalt message around
it.

This shouldn't be noisy, but if you think this isn't needed, it can go
away.

> 
> See, the world doesn't revolve around microcode loading. If that thing
> fails, then someone has done a bad job at the CPU vendor testing,
> provided the code does the right thing.
> 

When it fails due to current_rev < min_rev, Isn't it good to add indication
to user space that it didn't succeed? Thomas wanted these return codes, so
someone scripting can get a status after an attempt to load.

Otherwise agree, it shouldn't generally fail.

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Borislav Petkov]

On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> It's not an error, only when request_microcode() returns UCODE_ERROR, should
> it return -EINVAL,

So looking at ->request_microcode_fw(), it looks like we return
UCODE_ERROR when something with parsing the blob has gone wrong. So I
guess we can return something more fitting here to state that we failed
while parsing the microcode blob from userspace: it is corrupted,
truncated, what not.

Looking at the error codes, this:

#define ELIBBAD         80      /* Accessing a corrupted shared library */

seems fitting as it has "corrupted" blob in the definition. EBADF sounds
fitting too. In any case, it should be a distinct error value which
hints at what goes wrong.

> This shouldn't be noisy, but if you think this isn't needed, it can go
> away.

I think all this preemptive development - it might make sense so let's
do it - needs to stop. If there's an *actual* real use and need for it
sure, but let's issue a printk just because is not one of them.

> When it fails due to current_rev < min_rev, Isn't it good to add indication
> to user space that it didn't succeed? Thomas wanted these return codes, so
> someone scripting can get a status after an attempt to load.

Return codes: yes. Random, flaky, potentially overwritten in the dmesg
ring buffer error strings - nope. Soon someone will come along and say,
"hey, don't touch those printk formats - my tool parses them and it'll
break if you do." Yeah, right.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
> 
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
> 
> Looking at the error codes, this:
> 
> #define ELIBBAD         80      /* Accessing a corrupted shared library */
> 
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.

Perfect!. In the next respin, I can update this to EBADF. 

Later patches when we do minrev checks, or if the current rev is higher
than the one in the directory, what error would be fitting? Currently we do
EINVAL, I can't find a suitable one for that, if you have any good
suggestions that would be awesome.

> 
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
> 
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
> 
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
> 
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.

No arguments there. 

I'll drop the pr_warn() before taint as you suggest.

Appears we have good direction for patches 1-3. 

Cheers,
Ashok

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

Hi Boris,

While reworking I thought while at this, there is a chance to fix other
things.


On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
> 
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
> 
> Looking at the error codes, this:
> 
> #define ELIBBAD         80      /* Accessing a corrupted shared library */
> 
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.

Along the same lines, when check_online_cpu() should we return -EBUSY which
would distinguish between -EINVAL vs something different?

And we have a pr_err() to indicate not all CPUs are online. I suppose this
can be dropped in the same patch to fix return code? Since the error code
should be indicative of the problem?

pr_err("Not all CPUs online, aborting microcode update.\n");

> 
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
> 
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
> 
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
> 
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Borislav Petkov]

On Wed, Feb 01, 2023 at 01:47:58PM -0800, Ashok Raj wrote:
> While reworking I thought while at this, there is a chance to fix other
> things.

Your call. The more you're "fixing" other things, the longer it takes
for your minrev thing.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

On Wed, Feb 01, 2023 at 11:06:05PM +0100, Borislav Petkov wrote:
> On Wed, Feb 01, 2023 at 01:47:58PM -0800, Ashok Raj wrote:
> > While reworking I thought while at this, there is a chance to fix other
> > things.
> 
> Your call. The more you're "fixing" other things, the longer it takes
> for your minrev thing.

My original patch for minrev was 4 patches :-)

But when Thomas finds all the other cleanups, it turned into 9. And now
stuck in review.. 

I expected the change from apply_microcode() -> collect_cpu_info() to
be straight forward..

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Borislav Petkov]

On Wed, Feb 01, 2023 at 02:19:43PM -0800, Ashok Raj wrote:
> I expected the change from apply_microcode() -> collect_cpu_info() to
> be straight forward..

I expect a lot of things things too but reality is completely different.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Li, Aubrey]

On 2023/1/31 5:39, Ashok Raj wrote:
> Currently when late loading is aborted due to check_online_cpu(), kernel
> still ends up tainting the kernel.
> 
> Taint only when microcode loading was successful.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: x86 <x86@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Peter Zilstra (Intel) <peterz@infradead.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Martin Pohlack <mpohlack@amazon.de>
> ---
> v1->v2: (Thomas)
> 	- Remove unnecessary assignment of ret that's being overwritten.
> 	- Taint kernel only of loading was successful
> ---
>   arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 61d57d9b93ee..1c6831b8b244 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
>   	enum ucode_state tmp_ret = UCODE_OK;
>   	int bsp = boot_cpu_data.cpu_index;
>   	unsigned long val;
> -	ssize_t ret = 0;
> +	int load_ret = -1;
> +	ssize_t ret;
>   
>   	ret = kstrtoul(buf, 0, &val);
>   	if (ret)
> @@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
>   		goto put;
>   
>   	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> -	if (tmp_ret != UCODE_NEW)
> +	if (tmp_ret != UCODE_NEW) {
> +		ret = size;
>   		goto put;
> +	}
>   
>   	mutex_lock(&microcode_mutex);
> -	ret = microcode_reload_late();
> +	load_ret = microcode_reload_late();
>   	mutex_unlock(&microcode_mutex);
>   
>   put:
>   	cpus_read_unlock();
>   
> -	if (ret == 0)
> +	/*
> +	 * Taint only when loading was successful
> +	 */
> +	if (load_ret == 0) {
>   		ret = size;

What about if loading was not successful(load_ret != 0)?
ret has no chance to be returned as size here and we'll run into the 
endless update?

Thanks,
-Aubrey

> -
> -	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> +		pr_warn("Microcode late loading tainted the kernel\n");
> +	}
>   
>   	return ret;
>   }

Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful

[Ashok Raj]

On Tue, Jan 31, 2023 at 08:17:25PM +0800, Li, Aubrey wrote:
> On 2023/1/31 5:39, Ashok Raj wrote:
> > Currently when late loading is aborted due to check_online_cpu(), kernel
> > still ends up tainting the kernel.
> > 
> > Taint only when microcode loading was successful.
> > 

[snip]

> > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> > index 61d57d9b93ee..1c6831b8b244 100644
> > --- a/arch/x86/kernel/cpu/microcode/core.c
> > +++ b/arch/x86/kernel/cpu/microcode/core.c
> > @@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
> >   	enum ucode_state tmp_ret = UCODE_OK;
> >   	int bsp = boot_cpu_data.cpu_index;
> >   	unsigned long val;
> > -	ssize_t ret = 0;
> > +	int load_ret = -1;
> > +	ssize_t ret;
> >   	ret = kstrtoul(buf, 0, &val);
> >   	if (ret)
> > @@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
> >   		goto put;
> >   	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
> > -	if (tmp_ret != UCODE_NEW)
> > +	if (tmp_ret != UCODE_NEW) {
> > +		ret = size;
> >   		goto put;
> > +	}
> >   	mutex_lock(&microcode_mutex);
> > -	ret = microcode_reload_late();
> > +	load_ret = microcode_reload_late();
> >   	mutex_unlock(&microcode_mutex);
> >   put:
> >   	cpus_read_unlock();
> > -	if (ret == 0)
> > +	/*
> > +	 * Taint only when loading was successful
> > +	 */
> > +	if (load_ret == 0) {
> >   		ret = size;
> 
> What about if loading was not successful(load_ret != 0)?
> ret has no chance to be returned as size here and we'll run into the endless
> update?

Good catch, we'll need to make that some meaningful return code to stop the
endless wait.

[Patch v3 Part2 2/9] x86/microcode: Report invalid writes to reload sysfs file

[Ashok Raj]

Semantics of the microcode reload file are only defined if a "1" is
written. But the code silently treats any other unsigned integer as a
successful write even though no actions are performed to load microcode.

Report those erroneous writes back to user.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 1c6831b8b244..e4b4dfcf2d18 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -476,11 +476,8 @@ static ssize_t reload_store(struct device *dev,
 	ssize_t ret;
 
 	ret = kstrtoul(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	if (val != 1)
-		return size;
+	if (ret || val != 1)
+		return -EINVAL;
 
 	cpus_read_lock();
 
-- 
2.37.2

[tip: x86/microcode] x86/microcode: Allow only "1" as a late reload trigger value

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     25d0dc4b957cc8674f8554e85f18a00467e876d7
Gitweb:        https://git.kernel.org/tip/25d0dc4b957cc8674f8554e85f18a00467e876d7
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Mon, 30 Jan 2023 13:39:48 -08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Jan 2023 16:47:03 +01:00

x86/microcode: Allow only "1" as a late reload trigger value

Microcode gets reloaded late only if "1" is written to the reload file.
However, the code silently treats any other unsigned integer as a
successful write even though no actions are performed to load microcode.

Make the loader more strict to accept only "1" as a trigger value and
return an error otherwise.

  [ bp: Massage commit message. ]

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20230130213955.6046-3-ashok.raj@intel.com
---
 arch/x86/kernel/cpu/microcode/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 61d57d9..fdd1e7e 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -475,11 +475,8 @@ static ssize_t reload_store(struct device *dev,
 	ssize_t ret = 0;
 
 	ret = kstrtoul(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	if (val != 1)
-		return size;
+	if (ret || val != 1)
+		return -EINVAL;
 
 	cpus_read_lock();
 

[Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Ashok Raj]

Currently collect_cpu_info() is only returning what was cached earlier
instead of reading the current revision from the proper MSR.

Collect the current revision and report that value instead of reflecting
what was cached in the past.

[TBD:
    Need to change microcode/amd.c. I didn't quite follow the logic since
    it reports the revision from the patch file, instead of reporting the
    real PATCH_LEVEL MSR.

    Untested on AMD.
]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/kernel/cpu/microcode/intel.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 467cf37ea90a..de8e591c42cd 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -542,6 +542,13 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
 	unsigned int val[2];
+	int rev;
+
+	/*
+	 * intel_get_microcode_revision() reads a per-core MSR
+	 * to read the revision (MSR_IA32_UCODE_REV).
+	 */
+	WARN_ON_ONCE(cpu_num != smp_processor_id());
 
 	memset(csig, 0, sizeof(*csig));
 
@@ -553,7 +560,9 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 		csig->pf = 1 << ((val[1] >> 18) & 7);
 	}
 
-	csig->rev = c->microcode;
+	rev = intel_get_microcode_revision();
+	c->microcode = rev;
+	csig->rev = rev;
 
 	return 0;
 }
-- 
2.37.2

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Borislav Petkov]

On Mon, Jan 30, 2023 at 01:39:49PM -0800, Ashok Raj wrote:
> Currently collect_cpu_info() is only returning what was cached earlier
> instead of reading the current revision from the proper MSR.

Because this is how this is supposed to work: you collect what's
currently applied - which should be exactly the same as what's in the
MSR - and then see if there's a new patch in the cache and if so, update
it.

-- 
Regards/Gruss,
    Boris.

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

RE: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Luck, Tony]

>> Currently collect_cpu_info() is only returning what was cached earlier
>> instead of reading the current revision from the proper MSR.
>
> Because this is how this is supposed to work: you collect what's
> currently applied - which should be exactly the same as what's in the
> MSR - and then see if there's a new patch in the cache and if so, update
> it.

But those get out of step when applying ucode on one logical CPU does
an update to other(s) (in this case the HT sibling for the same core).

Would you prefer that Ashok leave collect_cpu_info() as-is, and create
a new function to read the MSR?

-Tony

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Ashok Raj]

On Tue, Jan 31, 2023 at 09:34:14AM -0800, Tony Luck wrote:
> >> Currently collect_cpu_info() is only returning what was cached earlier
> >> instead of reading the current revision from the proper MSR.
> >
> > Because this is how this is supposed to work: you collect what's
> > currently applied - which should be exactly the same as what's in the
> > MSR - and then see if there's a new patch in the cache and if so, update
> > it.
> 
> But those get out of step when applying ucode on one logical CPU does
> an update to other(s) (in this case the HT sibling for the same core).
> 
> Would you prefer that Ashok leave collect_cpu_info() as-is, and create
> a new function to read the MSR?
> 

Untested patch below. I think we could add a new function if needed as you
suggest.

Boris, do you have the same dependency to take the siblings through an
update to update some portions of the ucode? or they are only for early
loading?


diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index d144f918a896..0038690a5d4b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -673,8 +673,10 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	 * mc_bp_resume() can call apply_microcode()
 	 */
 	p = find_patch(cpu);
-	if (p && (p->patch_id == csig->rev))
+	if (p) {
+		csig->rev = c->microcode = ((struct microcode_amd *)p->data)->hdr.patch_id;
 		uci->mc = p->data;
+	}
 
 	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
 

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Borislav Petkov]

On Tue, Jan 31, 2023 at 05:34:14PM +0000, Luck, Tony wrote:
> But those get out of step when applying ucode on one logical CPU does
> an update to other(s) (in this case the HT sibling for the same core).

They shouldn't.

I presume you're talking about late update. If so and if it finds a
patch in the cache, it'll do this:

apply_microcode_intel:

        /*
         * Save us the MSR write below - which is a particular expensive
         * operation - when the other hyperthread has updated the microcode
         * already.
         */
        rev = intel_get_microcode_revision();
        if (rev >= mc->hdr.rev) {
                ret = UCODE_OK;
                goto out;
        }

and at the out: label it'll update the revision.

-- 
Regards/Gruss,
    Boris.

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

RE: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Luck, Tony]

> I presume you're talking about late update. If so and if it finds a
> patch in the cache, it'll do this:

Yes. This is about late update.

>
> apply_microcode_intel:
>
>         /*
>          * Save us the MSR write below - which is a particular expensive
>          * operation - when the other hyperthread has updated the microcode
>          * already.
>          */
>         rev = intel_get_microcode_revision();

What happens here if the update on the first hyperthread failed (sure, it shouldn't,
but stuff happens at large scale).  In this case the current rev is still older that the
the cache version ... so there is no "goto out", and this hyperthread will now write
the MSR to initiate microcode update here, while the first thread is off executing
arbitrary code (the situation that we want to avoid).

>         if (rev >= mc->hdr.rev) {
>                 ret = UCODE_OK;
>                 goto out;
>         }
>
> and at the out: label it'll update the revision.

-Tony

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Borislav Petkov]

On Tue, Jan 31, 2023 at 08:49:52PM +0000, Luck, Tony wrote:
> What happens here if the update on the first hyperthread failed (sure, it shouldn't,
> but stuff happens at large scale).  In this case the current rev is still older that the
> the cache version ... so there is no "goto out", and this hyperthread will now write
> the MSR to initiate microcode update here, while the first thread is off executing
> arbitrary code (the situation that we want to avoid).

Lemme see if I can follow: we sync all threads in __reload_late() and
once they all arrive, we send them down into ->apply_microcode.

T0 arrives, and fails the update. That is this piece:

        /* write microcode via MSR 0x79 */
        wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);

        rev = intel_get_microcode_revision();

        if (rev != mc->hdr.rev) {
                pr_err("CPU%d update to revision 0x%x failed\n",
                       cpu, mc->hdr.rev);
                return UCODE_ERROR;
        }

We return here without updating cpu_sig.rev, as we should.

T1 arrives, updates successfully and updates its cpu_sig.rev.

T0's patch level has been updated too with that because the microcode
engine is shared between the threads. T0's cpu_sig.rev isn't, however,
as that has happened "behind its back", so to speak.

Is that the scenario you're talking about?

If so, if you look at __reload_late(), it'll say

	pr_warn("Error reloading microcode on CPU %d\n", cpu);

and the large scale operator will know.

And well, the easy fix is, do the reload again. :-)

That'll update the cached values too.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Ashok Raj]

On Tue, Jan 31, 2023 at 10:08:48PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 08:49:52PM +0000, Luck, Tony wrote:
> > What happens here if the update on the first hyperthread failed (sure, it shouldn't,
> > but stuff happens at large scale).  In this case the current rev is still older that the
> > the cache version ... so there is no "goto out", and this hyperthread will now write
> > the MSR to initiate microcode update here, while the first thread is off executing
> > arbitrary code (the situation that we want to avoid).
> 
> Lemme see if I can follow: we sync all threads in __reload_late() and
> once they all arrive, we send them down into ->apply_microcode.

I was interpreting Thomas's response here

https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/

---------
#3

Not to talk about the completely broken error handling in the actual
microcode loading case in __reload_late()::wait_for_siblings code path.
---------

I thought sending sibling down the apply_microcode() just to update
revision might have bad interaction, so revise this to be more explicit and
update only the revision number and not have any other intended side
effect.

Patch3 was a prep-patch for patch4, to eliminate the call to
apply_microcode(). Maybe that wasn't the issue?

It's likely Thomas hinted at somthing else and I took the wrong hint.

> 
> T0 arrives, and fails the update. That is this piece:
> 
>         /* write microcode via MSR 0x79 */
>         wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
> 
>         rev = intel_get_microcode_revision();
> 
>         if (rev != mc->hdr.rev) {
>                 pr_err("CPU%d update to revision 0x%x failed\n",
>                        cpu, mc->hdr.rev);
>                 return UCODE_ERROR;
>         }
> 
> We return here without updating cpu_sig.rev, as we should.
> 
> T1 arrives, updates successfully and updates its cpu_sig.rev.
> 
> T0's patch level has been updated too with that because the microcode
> engine is shared between the threads. T0's cpu_sig.rev isn't, however,
> as that has happened "behind its back", so to speak.
> 
> Is that the scenario you're talking about?

The fix in patch4 was just attempting to not call apply_microcode(), since
at this time the other CPUs that arrived in wait_for_siblings: have left,
doing an update when the others are strictly not in renedzvous is the
condition we try to avoid.

Again, maybe this is me reading Thomas's request incorrectly.

Cheers,
Ashok

RE: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Luck, Tony]

> T0 arrives, and fails the update. That is this piece:
>
>         /* write microcode via MSR 0x79 */
>         wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
>
>         rev = intel_get_microcode_revision();
> 
>         if (rev != mc->hdr.rev) {
>                 pr_err("CPU%d update to revision 0x%x failed\n",
>                        cpu, mc->hdr.rev);
>                 return UCODE_ERROR;
>         }
>
> We return here without updating cpu_sig.rev, as we should.
>
> T1 arrives, updates successfully and updates its cpu_sig.rev.

In an ideal world yes. But what if T1 arrives here and tries to do the
update while T0, which has returned out of the microcode update
code and could be doing anything, happen to be doing WRMSR(some MSR
that the ucode update is tinkering with).

Now T0 explodes (not literally, I hope!) but does something crazy because
it was in the middle of some microcode flow that got updated between two
operations.

-Tony

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Borislav Petkov]

On Tue, Jan 31, 2023 at 10:43:23PM +0000, Luck, Tony wrote:
> In an ideal world yes. But what if T1 arrives here and tries to do the
> update while T0, which has returned out of the microcode update
> code and could be doing anything, happen to be doing WRMSR(some MSR
> that the ucode update is tinkering with).
> 
> Now T0 explodes (not literally, I hope!) but does something crazy because
> it was in the middle of some microcode flow that got updated between two
> operations.

So first of all, I'm wondering whether the scenario you're chasing is
something completely hypothetical or you're actually thinking of
something concrete which has actually happened or there's high potential
for it.

In that case, that late patching sync algorithm would need to be made
more robust to handle cases like that.

Because from what I'm reading above, this doesn't sound like the
reporting is wrong only but more like, if T0 fails the update and T1
gets to do that update for a change, then crap can happen.

Which means, our update dance cannot handle that case properly.

Hmmm...

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Ashok Raj]

On Wed, Feb 01, 2023 at 01:53:32PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 10:43:23PM +0000, Luck, Tony wrote:
> > In an ideal world yes. But what if T1 arrives here and tries to do the
> > update while T0, which has returned out of the microcode update
> > code and could be doing anything, happen to be doing WRMSR(some MSR
> > that the ucode update is tinkering with).
> > 
> > Now T0 explodes (not literally, I hope!) but does something crazy because
> > it was in the middle of some microcode flow that got updated between two
> > operations.
> 
> So first of all, I'm wondering whether the scenario you're chasing is
> something completely hypothetical or you're actually thinking of
> something concrete which has actually happened or there's high potential
> for it.
> 
> In that case, that late patching sync algorithm would need to be made
> more robust to handle cases like that.

That's correct. But fundamentally we sent the sibling down the
apply_microcode() path just to make sure the per-thread info is updated.

It appears the code is using a side effect that the revision got updated
even though we don't actually intend to perform a wrmsr on the sibling
in the normal case that primary completes the update.

If the purpose is only to update the revision, using the collect_cpu_info()
which seems more appropriate for that purpose, and doesn't have any
implied issues with using a wrmsr flow. It's not broken today, but the code
isn't future proof. Calling the revision update only keeps those questions
at bay.

I think this is what Thomas implied to cleanup in his comments. 

> 
> Because from what I'm reading above, this doesn't sound like the
> reporting is wrong only but more like, if T0 fails the update and T1
> gets to do that update for a change, then crap can happen.
> 
> Which means, our update dance cannot handle that case properly.
> 

It doesn't need to if we don't do an apply_microcode() for the sibling.

Cheers,
Ashok

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Borislav Petkov]

On Wed, Feb 01, 2023 at 07:13:30AM -0800, Ashok Raj wrote:
> If the purpose is only to update the revision, using the collect_cpu_info()
> which seems more appropriate for that purpose,

collect_cpu_info() is not called "update local revision" so no, it is
not appropriate.

I can't understand the rest of your handwaving so I'll give you the
usual spiel: first, define exactly what the real-life problem is you're
trying to solve.

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Luck, Tony]

> So first of all, I'm wondering whether the scenario you're chasing is
> something completely hypothetical or you're actually thinking of
> something concrete which has actually happened or there's high potential
> for it.

Rare, but very real. For one of the speculative side channel microcode
updates there were some reports from large data centers that the kernel
choked on a WRMSR(IA32_SPEC_CTRL)

> In that case, that late patching sync algorithm would need to be made
> more robust to handle cases like that.
"
Yup. when you get through the cleanups and the "minrev" update Ashok
has a handful of final[1] patches that add robustness.

-Tony

[1] Yes, there is an end in sight to all these microcode changes.

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Dave Hansen]

On 1/30/23 13:39, Ashok Raj wrote:
> Currently collect_cpu_info() is only returning what was cached earlier
> instead of reading the current revision from the proper MSR.
> 
> Collect the current revision and report that value instead of reflecting
> what was cached in the past.
> 
> [TBD:
>     Need to change microcode/amd.c. I didn't quite follow the logic since
>     it reports the revision from the patch file, instead of reporting the
>     real PATCH_LEVEL MSR.
> 
>     Untested on AMD.
> ]

This thread is meandering a bit.  I think it's because this changelog
doesn't have a problem statement.  It's hard to agree on a patch being a
solution to anything if we haven't first agreed on the problem.

What is the problem?

What does this "fix"?

Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info() to reflect current microcode

[Ashok Raj]

On Wed, Feb 01, 2023 at 11:13:58AM -0800, Dave Hansen wrote:
> On 1/30/23 13:39, Ashok Raj wrote:
> > Currently collect_cpu_info() is only returning what was cached earlier
> > instead of reading the current revision from the proper MSR.
> > 
> > Collect the current revision and report that value instead of reflecting
> > what was cached in the past.
> > 
> > [TBD:
> >     Need to change microcode/amd.c. I didn't quite follow the logic since
> >     it reports the revision from the patch file, instead of reporting the
> >     real PATCH_LEVEL MSR.
> > 
> >     Untested on AMD.
> > ]
> 
> This thread is meandering a bit.  I think it's because this changelog
> doesn't have a problem statement.  It's hard to agree on a patch being a
> solution to anything if we haven't first agreed on the problem.
> 
> What is the problem?

I alluded here.. But yes, clearly missed in the commit log.

https://lore.kernel.org/lkml/Y9mW7EiL%2FBpYFLWn@a4bf019067fa.jf.intel.com/

Thomas alluded here https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/
that error handling in __reload_late()::wait_for_siblings() code patch is
completely broken. 

This is one that I "assumed" he was referring to, since all we need is to
update the current revision, but we end up depending on the behavior of
apply_microcode() and that might accidentally have some side effects. 

Instead only call the collect_cpu_info() and allow that to update the
per-cpu revision instead. And there is no risk in performing that vs
accidentally letting it fall through with an apply_microcode() that might
have risks.

> 
> What does this "fix"?

The code performs this delicate late-load dance to prevent sibling threads
to be quiet while performing the update.

At wait_for_siblings() when all threads arrive, then the sibling does the
apply_microcode() which seems wrong. 

[Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Ashok Raj]

Microcode updates are applied at the core, so an update to one HT sibling
is effective on all HT siblings of the same core.

During late-load, after the primary has updated the microcode, it also
reflects that in the per-cpu structure (cpuinfo_x86) holding the current
revision.

Current code calls apply_microcode() to update the SW per-cpu revision.

But in the odd case when primary returned with an error, and as a result
the secondary didn't get the revision updated, will attempt to perform
a patch load and the primary has already been released to the system.
This could be problematic, because the whole rendezvous dance is to
prevent updates when one of the siblings could be executing arbitrary code.

Replace apply_microcode() with a call to collect_cpu_info() and let that
call also update the per-cpu structure instead of returning the previously
cached values.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index e4b4dfcf2d18..8452fad89bf6 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -386,6 +386,7 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
 static int __reload_late(void *info)
 {
 	int cpu = smp_processor_id();
+	struct ucode_cpu_info *uci;
 	enum ucode_state err;
 	int ret = 0;
 
@@ -421,12 +422,13 @@ static int __reload_late(void *info)
 
 	/*
 	 * At least one thread has completed update on each core.
-	 * For others, simply call the update to make sure the
-	 * per-cpu cpuinfo can be updated with right microcode
-	 * revision.
+	 * For siblings, collect the cpuinfo and update the
+	 * per-cpu cpuinfo with the current microcode revision.
 	 */
-	if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu)
-		err = microcode_ops->apply_microcode(cpu);
+	if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu) {
+		uci = ucode_cpu_info + cpu;
+		microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+	}
 
 	return ret;
 }
-- 
2.37.2

Re: [Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Dave Hansen]

On 1/30/23 13:39, Ashok Raj wrote:
> Microcode updates are applied at the core, so an update to one HT sibling
> is effective on all HT siblings of the same core.
> 
> During late-load, after the primary has updated the microcode, it also
> reflects that in the per-cpu structure (cpuinfo_x86) holding the current
> revision.
> 
> Current code calls apply_microcode() to update the SW per-cpu revision.

I'm having a hard time following this.  I can't even suggest a better
message because I can't grok this one.

> But in the odd case when primary returned with an error, and as a result
> the secondary didn't get the revision updated, will attempt to perform
> a patch load and the primary has already been released to the system.
> This could be problematic, because the whole rendezvous dance is to
> prevent updates when one of the siblings could be executing arbitrary code.

OK, let me see if I understand:

Today, ->apply_microcode() is called for both CPU threads.  Typically,
T0 comes in and will actually successfully update the microcode.  T1
will come in later, notice that T0 updated the microcode already and
return without even trying to do the update WRMSR.  One thing T1 _does_
do before returning is to update the per-cpu data.

That works great, unless T0 experiences an error.  In that case, T0 will
jump out of __reload_late() after failing to do the update.  T1 will
come bumbling along after it and will enter ->apply_microcode(),
blissfully unaware of T0's failure.  T1 will assume that it is supposed
to do T0's job, noting "rev < mc->hdr.rev".  T1 will write the MSR while
T0 is off doing god knows what.

T1 should not even be attempting to do ->apply_microcode() because T0 is
not quiescent.

> Replace apply_microcode() with a call to collect_cpu_info() and let that
> call also update the per-cpu structure instead of returning the previously
> cached values.

	To fix this, remove the path where T1 calls ->apply_microcode().
	However, this alone would leave the per-cpu metadata for T1 out
	of date.  Call collect_cpu_info() to ensure it is updated.

Right?

FWIW, this seems a bit hacky and inconsistent to me.  It would be nice
if the common T0/T1 work (updating the per-cpu metadata) was done with
common code.

Could we zap the uci->cpu_sig.rev work entirely from ->apply_microcode()
and do it in __reload_late() instead?

Re: [Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Borislav Petkov]

On Wed, Feb 01, 2023 at 02:21:18PM -0800, Dave Hansen wrote:
> That works great, unless T0 experiences an error.  In that case, T0 will
> jump out of __reload_late() after failing to do the update.  T1 will
> come bumbling along after it and will enter ->apply_microcode(),
> blissfully unaware of T0's failure.  T1 will assume that it is supposed
> to do T0's job, noting "rev < mc->hdr.rev".  T1 will write the MSR while
> T0 is off doing god knows what.
> 
> T1 should not even be attempting to do ->apply_microcode() because T0 is
> not quiescent.

Yah, thanks for explaining properly.

So, if T0 fails, then we will say that it failed. The ->apply_microcode()
call on T1 was never meant to apply any microcode - just to update the
cached data.

Now, if T0 fails, then it doesn't matter what T1 does - you have a
bigger problem:

A subset of the cores is running with new microcode while other subset
with the old one. Now this is a shit situation I don't want to be in.

And I don't have a good way out of it.

Revert to the old patch? Maybe...

Retry to application on all again with the hope that it works this time?

What if some core touches a MSR being added with the new microcode
patch?

Late loading is a big PITA. As we've been preaching for a while now.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Ashok Raj]

On Wed, Feb 01, 2023 at 11:40:58PM +0100, Borislav Petkov wrote:
> On Wed, Feb 01, 2023 at 02:21:18PM -0800, Dave Hansen wrote:
> > That works great, unless T0 experiences an error.  In that case, T0 will
> > jump out of __reload_late() after failing to do the update.  T1 will
> > come bumbling along after it and will enter ->apply_microcode(),
> > blissfully unaware of T0's failure.  T1 will assume that it is supposed
> > to do T0's job, noting "rev < mc->hdr.rev".  T1 will write the MSR while
> > T0 is off doing god knows what.
> > 
> > T1 should not even be attempting to do ->apply_microcode() because T0 is
> > not quiescent.
> 
> Yah, thanks for explaining properly.
> 
> So, if T0 fails, then we will say that it failed. The ->apply_microcode()
> call on T1 was never meant to apply any microcode - just to update the
> cached data.

The commit log "attempted" to convey that, replace primary with T0, and
secondary with T1.

> 
> Now, if T0 fails, then it doesn't matter what T1 does - you have a
> bigger problem:
> 
> A subset of the cores is running with new microcode while other subset
> with the old one. Now this is a shit situation I don't want to be in.
> 
> And I don't have a good way out of it.

T1 shouldn't be sent down the apply_microcode() path, but instead
just gather and update the per-cpu info reflecting the current revision.

Patch 3 and 4 attempted to that. 

In addition....to ensure cores being out of sync within themselves

At wait_for_siblings: Each thread can check their rev against the BSP (yes
BSP can be removed, but we can elect a core leader) and if they are
different we can either warn/taint or pull the plug and panic.


> 
> Revert to the old patch? Maybe...
> 
> Retry to application on all again with the hope that it works this time?
> 
> What if some core touches a MSR being added with the new microcode
> patch?
> 
> Late loading is a big PITA. As we've been preaching for a while now.
> 

Cheers,
Ashok

Re: [Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Borislav Petkov]

On Wed, Feb 01, 2023 at 06:51:44PM -0800, Ashok Raj wrote:
> T1 shouldn't be sent down the apply_microcode() path, but instead
> just gather and update the per-cpu info reflecting the current revision.

As I said previously, it doesn't apply the microcode but simply updates
the cached info. The assumption was that T0 would not fail.

> At wait_for_siblings: Each thread can check their rev against the BSP (yes
> BSP can be removed, but we can elect a core leader) and if they are

A core leader?

The one who succeeded in doing the update?

> different we can either warn/taint or pull the plug and panic.

What about SMT=off? How are you gonna handle that? Who's the core leader
there?

What about the other vendor? That hackery of yours needs to be verified
there too.

So this whole deal needs to be analyzed properly. What would happen in
any potential outcome, how should the kernel behave in each case, etc,
etc.

In thinking about the minrev, I can just as well imagine that such
microcode patches which could cause such a failure should be marked and
not loaded late either. But I'm sure you don't want that.

In any case, without a proper analysis and writeup how that new
algorithm is going to look like and behave, this is not going anywhere.
No ad-hoc patches, no let's fix that aspect first.

-- 
Regards/Gruss,
    Boris.

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

Re: [Patch v3 Part2 4/9] x86/microcode: Do not call apply_microcode() on sibling threads

[Ashok Raj]

Thanks Boris,

On Thu, Feb 02, 2023 at 10:40:31AM +0100, Borislav Petkov wrote:
> On Wed, Feb 01, 2023 at 06:51:44PM -0800, Ashok Raj wrote:
> > T1 shouldn't be sent down the apply_microcode() path, but instead
> > just gather and update the per-cpu info reflecting the current revision.
> 
> As I said previously, it doesn't apply the microcode but simply updates
> the cached info. The assumption was that T0 would not fail.

That is super clear. But the conclusion was, in the case T0 fails there are
some side effect that are hard to manage with the current algorighm.  

That was the motivation for the  change. Sending T1 to just collect and
report the revision, but not via apply_microcode() but via some alternate
function that doesn't have any bad interaction in the possible case if T0
failed.

> 
> > At wait_for_siblings: Each thread can check their rev against the BSP (yes
> > BSP can be removed, but we can elect a core leader) and if they are
> 
> A core leader?
> 
> The one who succeeded in doing the update?

My bad, core leader is the wrong term,  I meant something like how MCE
selects a rendezvous leader today. IRC, what we call as the monarch in
MCE land when it handles broadcast MCE's.

> 
> > different we can either warn/taint or pull the plug and panic.
> 
> What about SMT=off? How are you gonna handle that? Who's the core leader
> there?

This is broken today, IRC, we started to check cpu_present_mask and
cpu_online_mask counts are equal. This change to allow was a late change.

07d981ad4cf1 ("x86/microcode: Allow late microcode loading with SMT disabled")

Since those threads are in mwait(), I'm thinking for correctness we need to
revert that change.

Reverting the patch and borking late-load if ht=off was used, seems the
mitigation to prevent that.

Most BIOS's should have a similar ht_off, and that is the best way to turn
off HT. Its probably better that way to get all execution core resources
reserved for that one active thread.

> 
> What about the other vendor? That hackery of yours needs to be verified
> there too.

You mean updating the revision only outside of the path to
apply_microcode()? 

Its only AMD/Intel today for microcode updates. Yes we should test and
validate if this assumption is correct. But that's the *assumption* today
correct?

> 
> So this whole deal needs to be analyzed properly. What would happen in
> any potential outcome, how should the kernel behave in each case, etc,
> etc.

Complete aggreement. The one case we overlooked in the current algorithm
was "what if any core is in disaggreement with each other"

If there are other cases that come to mind, I can invest in looking into
that.

> 
> In thinking about the minrev, I can just as well imagine that such
> microcode patches which could cause such a failure should be marked and
> not loaded late either. But I'm sure you don't want that.

Once there is minrev enforcement,  default action, this is automatic,
since we refuse to load anything with minrev=0. 

They automatically become eligible for early load only. 

> 
> In any case, without a proper analysis and writeup how that new
> algorithm is going to look like and behave, this is not going anywhere.
> No ad-hoc patches, no let's fix that aspect first.

I'm happy to add more into Documentation/x86/microcode.rst. I can work with
Dave Hansen on closing or enumerating the algorithm and why we made the
choices. 

Cheers,
Ashok

[Patch v3 Part2 5/9] x86/microcode: Move late load warning to the same function that taints kernel

[Ashok Raj]

Late microcode loading issues a warning and taints the
kernel. Tainting the kernel and emitting the warning happens in two
different functions.

The upcoming support for safe late loading under certain conditions
needs to prevent both the warning and the tainting when the safe
conditions are met. That would require to hand the result of the safe
condition check into the function which emits the warning.

To avoid this awkward construct, move the warning into reload_store()
next to the taint() invocation as that is also the function which will
later contain the safe condition check.

No functional change.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 8452fad89bf6..bff566c05f46 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -442,9 +442,6 @@ static int microcode_reload_late(void)
 	int old = boot_cpu_data.microcode, ret;
 	struct cpuinfo_x86 prev_info;
 
-	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);
 
@@ -487,6 +484,9 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		goto put;
 
+	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");
+
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
 	if (tmp_ret != UCODE_NEW) {
 		ret = size;
-- 
2.37.2

[Patch v3 Part2 6/9] x86/microcode/intel: Add minimum required revision to microcode header

[Ashok Raj]

In general users don't have the necessary information to determine
whether a late loading of a new microcode version has removed any feature
(MSR, CPUID etc) between what is currently loaded and a new microcode
revision.  To address this issue, Intel has added a "minimum required
version" field to a previously reserved field in the microcode header.
Microcode updates should only be applied if the current microcode
version is equal to, or greater than this minimum required version.

Thomas made some suggestions[1] on how meta-data in the microcode file
could provide Linux with information to decide if the new microcode is
suitable candidate for late loading. But even the "simpler" option#1
requires a lot of metadata and corresponding kernel code to parse it.

Simply "OS visible features" such as CPUID and MSRs are the only two
examples. The microcode must not change these OS visible features
because they cause problems after late loading. When microcode changes
features, microcode will change the min_rev to prevent such microcodes
from being late loaded.

Pseudo code for late loading is as follows:

if header.min_required_id == 0
        This is old format microcode, block late loading
else if current_ucode_version < header.rev
	Abort update, can't update to older rev
else if current_ucode_version < header.min_required_id
        Current version is too old, block late loading of this microcode.
else
        OK to proceed with late loading.

Any microcode that modifies the interface to an OS-visible feature
will set the min_version to itself. This will enforce this microcode is
not suitable for late loading unless the currently loaded revision is
greater or equal to the new microcode affecting the change.

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.

When new features are added, there is no need for minrev enforcement.

Check if the new microcode specifies the minimum version for safe late
loading. Otherwise reject late load.

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] 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: Late loading denied: Current revision 0x8f685300 is too old to update, must be at 0xaa000050 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.

[1] https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
---
 arch/x86/include/asm/microcode_intel.h |  3 +-
 arch/x86/kernel/cpu/microcode/intel.c  | 39 +++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index f1fa979e05bf..e83afe919b10 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -15,7 +15,8 @@ struct microcode_header_intel {
 	unsigned int            datasize;
 	unsigned int            totalsize;
 	unsigned int            metasize;
-	unsigned int            reserved[2];
+	unsigned int		min_req_ver;
+	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 de8e591c42cd..4b3df85f2ca6 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -135,6 +135,38 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 		intel_ucode_patch = p->data;
 }
 
+static int is_lateload_safe(struct microcode_header_intel *mc_header)
+{
+	struct ucode_cpu_info uci;
+
+	/*
+	 * When late-loading, ensure the header declares a minimum revision
+	 * required to perform a late-load.
+	 */
+	if (!mc_header->min_req_ver) {
+		pr_warn("Late loading denied: Microcode header does not specify a required min version\n");
+		return -EINVAL;
+	}
+
+	intel_cpu_collect_info(&uci);
+
+	if (uci.cpu_sig.rev > mc_header->rev) {
+		pr_warn("Current microcode rev 0x%x greater than 0x%x, aborting\n",
+			uci.cpu_sig.rev, mc_header->rev);
+		return -EINVAL;
+	}
+	/*
+	 * Enforce the minimum revision specified in the header is either
+	 * greater or equal to the current revision.
+	 */
+	if (uci.cpu_sig.rev < mc_header->min_req_ver) {
+		pr_warn("Late loading denied: Current revision 0x%x too old to update, must be at 0x%x or higher. Use early loading instead\n",
+			uci.cpu_sig.rev, mc_header->min_req_ver);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * Get microcode matching with BSP's model. Only CPUs with the same model as
  * BSP can stay in the platform.
@@ -681,7 +713,9 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 		memcpy(mc, &mc_header, sizeof(mc_header));
 		data = mc + sizeof(mc_header);
 		if (!copy_from_iter_full(data, data_size, iter) ||
-		    intel_microcode_sanity_check(mc, true, MC_HEADER_TYPE_MICROCODE) < 0) {
+		    intel_microcode_sanity_check(mc, true, MC_HEADER_TYPE_MICROCODE) < 0 ||
+		    is_lateload_safe(&mc_header)) {
+			ret = UCODE_ERROR;
 			break;
 		}
 
@@ -704,6 +738,9 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 		return UCODE_ERROR;
 	}
 
+	if (ret == UCODE_ERROR)
+		return ret;
+
 	if (!new_mc)
 		return UCODE_NFOUND;
 
-- 
2.37.2

[Patch v3 Part2 7/9] x86/microcode: Add a generic mechanism to declare support for minrev

[Ashok Raj]

Intel microcode adds some meta-data to report a minimum required revision
before this new microcode can be safely late loaded. There are no generic
mechanism to declare support for all vendors.

Add generic support to microcode core to declare such support, this allows
late-loading to be permitted in those architectures that report support
for safe late loading.

Late loading has added support for

- New images declaring a required minimum base version before a late-load
  is performed.

Tainting only happens on architectures that don't support minimum required
version reporting.

Add a new variable in microcode_ops to allow an architecture to declare
support for safe microcode late loading.

Also make CONFIG_MICROCODE_LOADING by default, now that kernel enforces the
"minrev" requirement strictly.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/include/asm/microcode.h      |  2 ++
 arch/x86/kernel/cpu/microcode/core.c  | 26 +++++++++++++++++++++-----
 arch/x86/kernel/cpu/microcode/intel.c |  1 +
 arch/x86/Kconfig                      |  7 ++++---
 4 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d5a58bde091c..3d48143e84a9 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -33,6 +33,8 @@ enum ucode_state {
 };
 
 struct microcode_ops {
+	bool safe_late_load;
+
 	enum ucode_state (*request_microcode_fw) (int cpu, struct device *);
 
 	void (*microcode_fini_cpu) (int cpu);
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index bff566c05f46..be5d70396b79 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -470,6 +470,7 @@ static ssize_t reload_store(struct device *dev,
 {
 	enum ucode_state tmp_ret = UCODE_OK;
 	int bsp = boot_cpu_data.cpu_index;
+	bool safe_late_load = false;
 	unsigned long val;
 	int load_ret = -1;
 	ssize_t ret;
@@ -484,12 +485,25 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		goto put;
 
-	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");
+	safe_late_load = microcode_ops->safe_late_load;
+
+	/*
+	 * If safe loading indication isn't present, bail out.
+	 */
+	if (!safe_late_load) {
+		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");
+		ret = -EINVAL;
+		goto put;
+	}
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
 	if (tmp_ret != UCODE_NEW) {
-		ret = size;
+		/*
+		 * If loading fails for some other reason,
+		 * inform user appropriately
+		 */
+		ret = (tmp_ret == UCODE_ERROR) ? -EINVAL : size;
 		goto put;
 	}
 
@@ -505,8 +519,10 @@ static ssize_t reload_store(struct device *dev,
 	 */
 	if (load_ret == 0) {
 		ret = size;
-		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
-		pr_warn("Microcode late loading tainted the kernel\n");
+		if (!safe_late_load) {
+			add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+			pr_warn("Microcode late loading tainted the kernel\n");
+		}
 	}
 
 	return ret;
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 4b3df85f2ca6..98c92b9affa2 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -814,6 +814,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device)
 }
 
 static struct microcode_ops microcode_intel_ops = {
+	.safe_late_load			  = true,
 	.request_microcode_fw             = request_microcode_fw,
 	.collect_cpu_info                 = collect_cpu_info,
 	.apply_microcode                  = apply_microcode_intel,
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3604074a878b..ddc4130e6f8c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1352,15 +1352,16 @@ config MICROCODE_AMD
 	  processors will be enabled.
 
 config MICROCODE_LATE_LOADING
-	bool "Late microcode loading (DANGEROUS)"
-	default n
+	bool "Late microcode loading"
+	default y
 	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.
+	  use this at your own risk. Late loading taints the kernel, if it
+	  doesn't support a minimum required base version before an update.
 
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
-- 
2.37.2

[Patch v3 Part2 8/9] x86/microcode/intel: Drop wbinvd() from microcode loading

[Ashok Raj]

Some older processors had a bad interaction when updating microcode if the
caches were dirty causing machine checks. The wbinvd() was added to
mitigate that before performing microcode updates. Now that Linux checks
for the minimum version before performing an update, those microcode
revisions can't be loaded.

Early loading is also not required to use wbinvd() any longer, that was
added as a safety net.

Remove calls to wbinvd().

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Martin Pohlack <mpohlack@amazon.de>
---
 arch/x86/kernel/cpu/microcode/intel.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 98c92b9affa2..601c586be7b6 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -415,12 +415,6 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 
 	old_rev = rev;
 
-	/*
-	 * Writeback and invalidate caches before updating microcode to avoid
-	 * internal issues depending on what the microcode is updating.
-	 */
-	native_wbinvd();
-
 	/* write microcode via MSR 0x79 */
 	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
@@ -632,12 +626,6 @@ static enum ucode_state apply_microcode_intel(int cpu)
 		goto out;
 	}
 
-	/*
-	 * Writeback and invalidate caches before updating microcode to avoid
-	 * internal issues depending on what the microcode is updating.
-	 */
-	native_wbinvd();
-
 	/* write microcode via MSR 0x79 */
 	wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
-- 
2.37.2

[Patch v3 Part2 9/9] x86/microcode: Provide an option to override minrev enforcement

[Ashok Raj]

Minimum Required Revision (minrev) is enforced strictly. All new patches
will have a minrev that is not zero. But there might be a transition time
for some that need this enforcement to be relaxed.

When the override is enabled, the kernel will be tainted.

Provide a debugfs variable to override the minrev enforcement.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner (Intel) <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Stefan Talpalaru <stefantalpalaru@yahoo.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Peter Zilstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
---
 arch/x86/include/asm/microcode.h      |  2 ++
 arch/x86/kernel/cpu/microcode/core.c  | 15 +++++++++++++--
 arch/x86/kernel/cpu/microcode/intel.c |  8 ++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 3d48143e84a9..d82f22d50ebd 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -16,6 +16,8 @@ struct ucode_patch {
 
 extern struct list_head microcode_cache;
 
+extern bool override_minrev;
+
 struct cpu_signature {
 	unsigned int sig;
 	unsigned int pf;
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index be5d70396b79..dbcccbd46ab8 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -23,6 +23,7 @@
 #include <linux/miscdevice.h>
 #include <linux/capability.h>
 #include <linux/firmware.h>
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
@@ -43,7 +44,9 @@
 #define DRIVER_VERSION	"2.2"
 
 static struct microcode_ops	*microcode_ops;
+static struct dentry		*dentry_ucode;
 static bool dis_ucode_ldr = true;
+bool override_minrev;
 
 bool initrd_gone;
 
@@ -494,7 +497,11 @@ static ssize_t reload_store(struct device *dev,
 		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");
 		ret = -EINVAL;
-		goto put;
+
+		if (!override_minrev)
+			goto put;
+
+		pr_info("Overriding minrev\n");
 	}
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
@@ -519,7 +526,7 @@ static ssize_t reload_store(struct device *dev,
 	 */
 	if (load_ret == 0) {
 		ret = size;
-		if (!safe_late_load) {
+		if (!safe_late_load || override_minrev) {
 			add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 			pr_warn("Microcode late loading tainted the kernel\n");
 		}
@@ -692,7 +699,11 @@ static int __init microcode_init(void)
 	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
 				  mc_cpu_online, mc_cpu_down_prep);
 
+	dentry_ucode = debugfs_create_dir("microcode", NULL);
+	debugfs_create_bool("override_minrev", 0644, dentry_ucode, &override_minrev);
+
 	pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
+	pr_info("Override minrev %s\n", override_minrev ? "enabled" : "disabled");
 
 	return 0;
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 601c586be7b6..ec5a29ebee8e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -139,6 +139,14 @@ static int is_lateload_safe(struct microcode_header_intel *mc_header)
 {
 	struct ucode_cpu_info uci;
 
+	/*
+	 * If minrev is bypassed via debugfs, then allow late-load.
+	 */
+	if (override_minrev) {
+		pr_info("Bypassing minrev enforcement via debugfs\n");
+		return 0;
+	}
+
 	/*
 	 * When late-loading, ensure the header declares a minimum revision
 	 * required to perform a late-load.
-- 
2.37.2