[PATCH 0/6] VERW based clean-up

[PATCH 0/6] VERW based clean-up

[Daniel Sneddon]

There are several mitigations that use the VERW instruction to
clean up internal CPU buffers.  Currently, each of these mitigations is
treated independently, but if VERW is needed for one of the
mitigations, it's on for all of them. This can lead to some confusion
if a user tries to disable one of the mitigations, but it is left
enabled for one of the others. The user would have to disable all 4
VERW based mitigations.  This series aims to make it easier for the
end user to disable VERW based mitigations by introducing a single
parameter to control all of them. This makes the individual parameters
redundant, so they are removed. It may be desirable to keep the old
individual parameters for the sake of backwards compatibility.

Daniel Sneddon (6):
  x86/bugs: Create single parameter for VERW based mitigations
  x86/bugs: Remove MDS command line
  x86/bugs: Remove TAA kernel parameter.
  x86/bugs: Remove MMIO kernel parameter
  x86/bugs: Remove RFDS kernel parameter.
  x86/bugs: Clean-up verw mitigations

 .../admin-guide/kernel-parameters.txt         | 143 +--------
 arch/x86/kernel/cpu/bugs.c                    | 287 +++++++-----------
 2 files changed, 118 insertions(+), 312 deletions(-)

-- 
2.25.1

[PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

There are currently 4 mitigations that use VERW to flush different cpu
buffers. This can cause confusion when trying to disable all the
different VERW mitigations. Simplify enabling/disabling these
mitigations by creating a single parameter for controlling them.

Future work will focus on combining similar code used in selecting
these mitigations to further simplify.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         | 16 +++++++++
 arch/x86/kernel/cpu/bugs.c                    | 34 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 09126bb8cc9f..66b567c4dce5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -628,6 +628,21 @@
 	cio_ignore=	[S390]
 			See Documentation/arch/s390/common_io.rst for details.
 
+	clear_cpu_buffers=
+			[X86]
+			Controls the mitigations that use
+			X86_FEATURE_CLEAR_CPU_BUF, namely
+			Micro-architectrual Data Sampling (MDS)
+			MMIO Stale Data
+			TSX Async Abort (TAA)
+			Register File Data Sampling (RFDS)
+
+			The options are:
+			on		- Enable cpu buffer clearing
+			on,nosmt	- Enable cpu buffer clearing and disable
+					  SMT
+			off		- Disables cpu buffer clearing
+
 	clearcpuid=X[,X...] [X86]
 			Disable CPUID feature X for the kernel. See
 			arch/x86/include/asm/cpufeatures.h for the valid bit
@@ -3461,6 +3476,7 @@
 				improves system performance, but it may also
 				expose users to several CPU vulnerabilities.
 				Equivalent to: if nokaslr then kpti=0 [ARM64]
+					       clear_cpu_buffers=off [X86]
 					       gather_data_sampling=off [X86]
 					       kvm.nx_huge_pages=off [X86]
 					       l1tf=off [X86]
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 45675da354f3..b3c9e1eede12 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -531,6 +531,40 @@ early_param("reg_file_data_sampling", rfds_parse_cmdline);
 #undef pr_fmt
 #define pr_fmt(fmt)     "" fmt
 
+static int __init clear_cpu_buffers_cmdline(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	if (!boot_cpu_has_bug(X86_BUG_MDS) &&
+	    !boot_cpu_has_bug(X86_BUG_TAA) &&
+	    !boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) &&
+	    !boot_cpu_has_bug(X86_BUG_RFDS))
+		return 0;
+
+	if (!strcmp(str, "off")) {
+		mds_mitigation = MDS_MITIGATION_OFF;
+		taa_mitigation = TAA_MITIGATION_OFF;
+		mmio_mitigation = MMIO_MITIGATION_OFF;
+		rfds_mitigation = RFDS_MITIGATION_OFF;
+	} else if (!strcmp(str, "on")) {
+		mds_mitigation = MDS_MITIGATION_FULL;
+		taa_mitigation = TAA_MITIGATION_VERW;
+		mmio_mitigation = MMIO_MITIGATION_VERW;
+		rfds_mitigation = RFDS_MITIGATION_VERW;
+	} else if (!strcmp(str, "on,nosmt")) {
+		mds_mitigation = MDS_MITIGATION_FULL;
+		taa_mitigation = TAA_MITIGATION_VERW;
+		mmio_mitigation = MMIO_MITIGATION_VERW;
+		rfds_mitigation = RFDS_MITIGATION_VERW;
+		mds_nosmt = true;
+		taa_nosmt = true;
+		mmio_nosmt = true;
+	}
+	return 0;
+}
+early_param("clear_cpu_buffers", clear_cpu_buffers_cmdline);
+
 static void __init md_clear_update_mitigation(void)
 {
 	if (cpu_mitigations_off())
-- 
2.25.1

RE: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Kaplan, David]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> Sent: Tuesday, September 24, 2024 5:32 PM
> To: Jonathan Corbet <corbet@lwn.net>; Thomas Gleixner <tglx@linutronix.de>;
> Borislav Petkov <bp@alien8.de>; Peter Zijlstra <peterz@infradead.org>; Josh
> Poimboeuf <jpoimboe@kernel.org>; Ingo Molnar <mingo@redhat.com>; Dave
> Hansen <dave.hansen@linux.intel.com>; x86@kernel.org
> Cc: hpa@zytor.com; linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
> pawan.kumar.gupta@linux.intel.com
> Subject: [PATCH 1/6] x86/bugs: Create single parameter for VERW based
> mitigations
>
> There are currently 4 mitigations that use VERW to flush different cpu buffers. This
> can cause confusion when trying to disable all the different VERW mitigations.
> Simplify enabling/disabling these mitigations by creating a single parameter for
> controlling them.

Just curious, what is the use case for disabling the different VERW mitigations (but not other mitigations)?  Is that a testing/debugging use case or a production use case?

>
> Future work will focus on combining similar code used in selecting these mitigations
> to further simplify.
>
> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 16 +++++++++
>  arch/x86/kernel/cpu/bugs.c                    | 34 +++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 09126bb8cc9f..66b567c4dce5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -628,6 +628,21 @@
>       cio_ignore=     [S390]
>                       See Documentation/arch/s390/common_io.rst for details.
>
> +     clear_cpu_buffers=
> +                     [X86]
> +                     Controls the mitigations that use
> +                     X86_FEATURE_CLEAR_CPU_BUF, namely
> +                     Micro-architectrual Data Sampling (MDS)
> +                     MMIO Stale Data
> +                     TSX Async Abort (TAA)
> +                     Register File Data Sampling (RFDS)
> +
> +                     The options are:
> +                     on              - Enable cpu buffer clearing
> +                     on,nosmt        - Enable cpu buffer clearing and disable
> +                                       SMT
> +                     off             - Disables cpu buffer clearing
> +

At the x86 uconf at LPC, someone asked me about if we should have command line options that were mitigation-focused rather than bug-focused (like to enable STIBP, IBRS, etc.).  The feedback I had there applies to this series too, which is that I'm concerned this makes things more difficult for users because users are reacting to bugs, they're not experts in mitigations.  A user wants to know how to mitigate CVE XYZ, and the bug-specific command line options support that.  It's an extra step to say that to mitigate MDS, you have to figure out that MDS requires clearing cpu buffers, and therefore you should set this mitigation-specific option.

My general concern with this series is it seems to tie X86_FEATURE_CLEAR_CPU_BUF directly to these 4 bugs.  What would happen if hypothetically there was a new bug that required X86_FEATURE_CLEAR_CPU_BUF and some other mitigation?  With the existing bug-specific options this is easy enough, as the new bug could force this feature and do whatever else it needed.  But with a mitigation-specific option like this one, it would seem to be harder as it might require multiple options to mitigate one bug.  And could create conflicts if you enable that new mitigation but disable clear_cpu_buffers.

I do get the point that these specific 4 bugs are all closely related.  Another idea to consider could be a single command line option for these 4 bugs, but is tied to the bugs themselves, not the mitigation.  That might be more future-proof as the scope remains only about these 4 bugs, not the mitigation itself.

--David Kaplan

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

On 10/8/24 12:24, Kaplan, David wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
>> Sent: Tuesday, September 24, 2024 5:32 PM
>> To: Jonathan Corbet <corbet@lwn.net>; Thomas Gleixner <tglx@linutronix.de>;
>> Borislav Petkov <bp@alien8.de>; Peter Zijlstra <peterz@infradead.org>; Josh
>> Poimboeuf <jpoimboe@kernel.org>; Ingo Molnar <mingo@redhat.com>; Dave
>> Hansen <dave.hansen@linux.intel.com>; x86@kernel.org
>> Cc: hpa@zytor.com; linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
>> pawan.kumar.gupta@linux.intel.com
>> Subject: [PATCH 1/6] x86/bugs: Create single parameter for VERW based
>> mitigations
>>
>> There are currently 4 mitigations that use VERW to flush different cpu buffers. This
>> can cause confusion when trying to disable all the different VERW mitigations.
>> Simplify enabling/disabling these mitigations by creating a single parameter for
>> controlling them.
> 
> Just curious, what is the use case for disabling the different VERW mitigations (but not other mitigations)?  Is that a testing/debugging use case or a production use case?
> 
>>
>> Future work will focus on combining similar code used in selecting these mitigations
>> to further simplify.
>>
>> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         | 16 +++++++++
>>  arch/x86/kernel/cpu/bugs.c                    | 34 +++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index 09126bb8cc9f..66b567c4dce5 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -628,6 +628,21 @@
>>       cio_ignore=     [S390]
>>                       See Documentation/arch/s390/common_io.rst for details.
>>
>> +     clear_cpu_buffers=
>> +                     [X86]
>> +                     Controls the mitigations that use
>> +                     X86_FEATURE_CLEAR_CPU_BUF, namely
>> +                     Micro-architectrual Data Sampling (MDS)
>> +                     MMIO Stale Data
>> +                     TSX Async Abort (TAA)
>> +                     Register File Data Sampling (RFDS)
>> +
>> +                     The options are:
>> +                     on              - Enable cpu buffer clearing
>> +                     on,nosmt        - Enable cpu buffer clearing and disable
>> +                                       SMT
>> +                     off             - Disables cpu buffer clearing
>> +
> 
> At the x86 uconf at LPC, someone asked me about if we should have command line options that were mitigation-focused rather than bug-focused (like to enable STIBP, IBRS, etc.).  The feedback I had there applies to this series too, which is that I'm concerned this makes things more difficult for users because users are reacting to bugs, they're not experts in mitigations.  A user wants to know how to mitigate CVE XYZ, and the bug-specific command line options support that.  It's an extra step to say that to mitigate MDS, you have to figure out that MDS requires clearing cpu buffers, and therefore you should set this mitigation-specific option.
> 
> My general concern with this series is it seems to tie X86_FEATURE_CLEAR_CPU_BUF directly to these 4 bugs.  What would happen if hypothetically there was a new bug that required X86_FEATURE_CLEAR_CPU_BUF and some other mitigation?  With the existing bug-specific options this is easy enough, as the new bug could force this feature and do whatever else it needed.  But with a mitigation-specific option like this one, it would seem to be harder as it might require multiple options to mitigate one bug.  And could create conflicts if you enable that new mitigation but disable clear_cpu_buffers.
> 

Any new bug that is using X86_FEATURE_CLEAR_CPU_BUF will be related to these
existing bugs regardless. We may need to add another option to this parameter,
similar to what we do with ",nosmt", but I would hope that would be sufficient.
With the existing bug-focused parameters we have the same potential for
conflicts. What if I say "reg_file_data_sampling=off mds=full"? Since mds is on
rfds will be on even though I requested it off. The intent of this parameter is
to remove such conflicts.

> I do get the point that these specific 4 bugs are all closely related.  Another idea to consider could be a single command line option for these 4 bugs, but is tied to the bugs themselves, not the mitigation.  That might be more future-proof as the scope remains only about these 4 bugs, not the mitigation itself.
> 

Are you suggesting a name change away from "clear_cpu_buffers" since it is
clearly about the mitigation rather than the bug? I'm not sure there is a good
common name for those 4 bugs that isn't about the mitigation, but I'm open to
any suggestions.

> --David Kaplan

RE: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Kaplan, David]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> Sent: Wednesday, October 9, 2024 11:18 AM
> To: Kaplan, David <David.Kaplan@amd.com>; Jonathan Corbet
> <corbet@lwn.net>; Thomas Gleixner <tglx@linutronix.de>; Borislav Petkov
> <bp@alien8.de>; Peter Zijlstra <peterz@infradead.org>; Josh Poimboeuf
> <jpoimboe@kernel.org>; Ingo Molnar <mingo@redhat.com>; Dave Hansen
> <dave.hansen@linux.intel.com>; x86@kernel.org
> Cc: hpa@zytor.com; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; pawan.kumar.gupta@linux.intel.com
> Subject: Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based
> mitigations
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
> >
> > At the x86 uconf at LPC, someone asked me about if we should have
> command line options that were mitigation-focused rather than bug-focused
> (like to enable STIBP, IBRS, etc.).  The feedback I had there applies to this
> series too, which is that I'm concerned this makes things more difficult for
> users because users are reacting to bugs, they're not experts in mitigations.
> A user wants to know how to mitigate CVE XYZ, and the bug-specific
> command line options support that.  It's an extra step to say that to mitigate
> MDS, you have to figure out that MDS requires clearing cpu buffers, and
> therefore you should set this mitigation-specific option.
> >
> > My general concern with this series is it seems to tie
> X86_FEATURE_CLEAR_CPU_BUF directly to these 4 bugs.  What would
> happen if hypothetically there was a new bug that required
> X86_FEATURE_CLEAR_CPU_BUF and some other mitigation?  With the
> existing bug-specific options this is easy enough, as the new bug could force
> this feature and do whatever else it needed.  But with a mitigation-specific
> option like this one, it would seem to be harder as it might require multiple
> options to mitigate one bug.  And could create conflicts if you enable that
> new mitigation but disable clear_cpu_buffers.
> >
>
> Any new bug that is using X86_FEATURE_CLEAR_CPU_BUF will be related to
> these existing bugs regardless. We may need to add another option to this
> parameter, similar to what we do with ",nosmt", but I would hope that
> would be sufficient.
> With the existing bug-focused parameters we have the same potential for
> conflicts. What if I say "reg_file_data_sampling=off mds=full"? Since mds is
> on rfds will be on even though I requested it off. The intent of this parameter
> is to remove such conflicts.
>
> > I do get the point that these specific 4 bugs are all closely related.  Another
> idea to consider could be a single command line option for these 4 bugs, but
> is tied to the bugs themselves, not the mitigation.  That might be more
> future-proof as the scope remains only about these 4 bugs, not the
> mitigation itself.
> >
>
> Are you suggesting a name change away from "clear_cpu_buffers" since it is
> clearly about the mitigation rather than the bug? I'm not sure there is a good
> common name for those 4 bugs that isn't about the mitigation, but I'm open
> to any suggestions.
>

Yes, I think that would be better.  I wasn't sure on a name either.  In the RFDS webpage I see it described as "similar to data sampling transient execution attacks".  Perhaps something like that could be an umbrella term?

--David Kaplan

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

>>
>> Are you suggesting a name change away from "clear_cpu_buffers" since it is
>> clearly about the mitigation rather than the bug? I'm not sure there is a good
>> common name for those 4 bugs that isn't about the mitigation, but I'm open
>> to any suggestions.
>>
> 
> Yes, I think that would be better.  I wasn't sure on a name either.  In the RFDS webpage I see it described as "similar to data sampling transient execution attacks".  Perhaps something like that could be an umbrella term?
> 

Sure, I can change it. Thanks for the review!

> --David Kaplan

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

On 10/9/24 09:39, Daniel Sneddon wrote:
> 
>>>
>>> Are you suggesting a name change away from "clear_cpu_buffers" since it is
>>> clearly about the mitigation rather than the bug? I'm not sure there is a good
>>> common name for those 4 bugs that isn't about the mitigation, but I'm open
>>> to any suggestions.
>>>
>>
>> Yes, I think that would be better.  I wasn't sure on a name either.  In the RFDS webpage I see it described as "similar to data sampling transient execution attacks".  Perhaps something like that could be an umbrella term?
>>

data sampling feels a little too generic. How about something like
microarch_data_sampling?

RE: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Kaplan, David]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> Sent: Wednesday, October 9, 2024 2:44 PM
> To: Kaplan, David <David.Kaplan@amd.com>; Jonathan Corbet
> <corbet@lwn.net>; Thomas Gleixner <tglx@linutronix.de>; Borislav Petkov
> <bp@alien8.de>; Peter Zijlstra <peterz@infradead.org>; Josh Poimboeuf
> <jpoimboe@kernel.org>; Ingo Molnar <mingo@redhat.com>; Dave Hansen
> <dave.hansen@linux.intel.com>; x86@kernel.org
> Cc: hpa@zytor.com; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; pawan.kumar.gupta@linux.intel.com
> Subject: Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based
> mitigations
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 10/9/24 09:39, Daniel Sneddon wrote:
> >
> >>>
> >>> Are you suggesting a name change away from "clear_cpu_buffers" since
> >>> it is clearly about the mitigation rather than the bug? I'm not sure
> >>> there is a good common name for those 4 bugs that isn't about the
> >>> mitigation, but I'm open to any suggestions.
> >>>
> >>
> >> Yes, I think that would be better.  I wasn't sure on a name either.  In the
> RFDS webpage I see it described as "similar to data sampling transient
> execution attacks".  Perhaps something like that could be an umbrella term?
> >>
>
> data sampling feels a little too generic. How about something like
> microarch_data_sampling?

That's what MDS stands for right?  Might be too associated with that one issue.

Maybe transient_data_sampling?

--David Kaplan

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

On 10/9/24 13:02, Kaplan, David wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
>> Sent: Wednesday, October 9, 2024 2:44 PM
>> To: Kaplan, David <David.Kaplan@amd.com>; Jonathan Corbet
>> <corbet@lwn.net>; Thomas Gleixner <tglx@linutronix.de>; Borislav Petkov
>> <bp@alien8.de>; Peter Zijlstra <peterz@infradead.org>; Josh Poimboeuf
>> <jpoimboe@kernel.org>; Ingo Molnar <mingo@redhat.com>; Dave Hansen
>> <dave.hansen@linux.intel.com>; x86@kernel.org
>> Cc: hpa@zytor.com; linux-doc@vger.kernel.org; linux-
>> kernel@vger.kernel.org; pawan.kumar.gupta@linux.intel.com
>> Subject: Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based
>> mitigations
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> On 10/9/24 09:39, Daniel Sneddon wrote:
>>>
>>>>>
>>>>> Are you suggesting a name change away from "clear_cpu_buffers" since
>>>>> it is clearly about the mitigation rather than the bug? I'm not sure
>>>>> there is a good common name for those 4 bugs that isn't about the
>>>>> mitigation, but I'm open to any suggestions.
>>>>>
>>>>
>>>> Yes, I think that would be better.  I wasn't sure on a name either.  In the
>> RFDS webpage I see it described as "similar to data sampling transient
>> execution attacks".  Perhaps something like that could be an umbrella term?
>>>>
>>
>> data sampling feels a little too generic. How about something like
>> microarch_data_sampling?
> 
> That's what MDS stands for right?  Might be too associated with that one issue.

Oops. Right. I'm so used to the acronym I can't even remember what it actually
stands for! :)

> 
> Maybe transient_data_sampling?
> 
That'll work! Thanks!
> --David Kaplan

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Josh Poimboeuf]

On Tue, Oct 08, 2024 at 07:24:01PM +0000, Kaplan, David wrote:
> > There are currently 4 mitigations that use VERW to flush different cpu buffers. This
> > can cause confusion when trying to disable all the different VERW mitigations.
> > Simplify enabling/disabling these mitigations by creating a single parameter for
> > controlling them.
> 
> Just curious, what is the use case for disabling the different VERW
> mitigations (but not other mitigations)?  Is that a testing/debugging
> use case or a production use case?

I'm also wondering about this.  I like the idea of cleaning up the code
mess (like in patch 6) but I'm not sure I see the benefit of this patch
to add yet another option.

Is this a realistic use case?  Are people really going to want to
enable/disable VERW mitigations as a group?

-- 
Josh

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Borislav Petkov]

On Wed, Oct 09, 2024 at 09:52:19PM -0700, Josh Poimboeuf wrote:
> Is this a realistic use case?  Are people really going to want to
> enable/disable VERW mitigations as a group?

+1.

David's per-attack-vector stuff will simplify the user side of this
considerably so I'm trying real-hard to find the point for a new option.

IOW, the reason I requested this cleanup is to have proper sync between the
different mitigations all using VERW behind the scenes. But there's no need to
change the user interface, is it?

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

On 10/10/24 07:57, Borislav Petkov wrote:
> On Wed, Oct 09, 2024 at 09:52:19PM -0700, Josh Poimboeuf wrote:
>> Is this a realistic use case?  Are people really going to want to
>> enable/disable VERW mitigations as a group?

They have to. The way you do it today is by setting four different options. If
you miss one and your system has the bug you missed, too bad, you're getting the
mitigation enabled. Since we have four bugs but only one mitigation, I thought
it made more sense to just have 1 knob to control it rather than 4. However,
since we'd need to keep those old knobs around anyway it turns out we'd just
have 5. :( <insert XKCD comic here>

> 
> +1.
> 
> David's per-attack-vector stuff will simplify the user side of this
> considerably so I'm trying real-hard to find the point for a new option.
> 
> IOW, the reason I requested this cleanup is to have proper sync between the
> different mitigations all using VERW behind the scenes. But there's no need to
> change the user interface, is it?
> 

The reason I did the patches this way wasn't so much "need" as it just seemed a
simpler way to do it. Why have 4 knobs when there is really only 1 mitigation
under the hood? My question for you then is what you mean by "proper sync"? I'm
guessing you mean that if any one of those 4 mitigations is set to off then
assume all are off? No one should want to set say, MMIO to =off but RFDS to =on,
so the only real issue is if I set some to =off, but leave others unset, the
unspecified options will default to on, which means all are on. If the desire is
to reverse that so any one of the 4 being disabled is enough to disable all VERW
mitigations, I can make that change. I just want to make sure I know what the
desired path is.

Thanks,
Dan

> Thx.
> 

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Borislav Petkov]

On Mon, Oct 14, 2024 at 08:42:26AM -0700, Daniel Sneddon wrote:
> The reason I did the patches this way wasn't so much "need" as it just seemed a
> simpler way to do it. Why have 4 knobs when there is really only 1 mitigation
> under the hood? My question for you then is what you mean by "proper sync"? I'm
> guessing you mean that if any one of those 4 mitigations is set to off then
> assume all are off? 

Well, up until now at least, we have handled under the assumption that not
every user knows exactly what needs to be configured in order to be safe.

So, we have always aimed for a sane default.

IOW, if a user wants to disable one mitigation but all 4 are mitigated by the
same thing, then we probably should issue a warning saying something like:

	"If you want to disable W, then you need to disable W, X and Y too in
	order to disable W effectively as all 4 are mitigated by the same
	mechanism."

And problem solved.

IOW, I don't expect someone would consciously want to disable a subset of
those mitigations but leave the remaining ones on. What usually happens, is
people do "mitigations=off" in order to regain their performance but not do
this selective thing which doesn't make a whole lot sense to me anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/6] x86/bugs: Create single parameter for VERW based mitigations

[Daniel Sneddon]

On 10/15/24 06:52, Borislav Petkov wrote:
> On Mon, Oct 14, 2024 at 08:42:26AM -0700, Daniel Sneddon wrote:
>> The reason I did the patches this way wasn't so much "need" as it just seemed a
>> simpler way to do it. Why have 4 knobs when there is really only 1 mitigation
>> under the hood? My question for you then is what you mean by "proper sync"? I'm
>> guessing you mean that if any one of those 4 mitigations is set to off then
>> assume all are off? 
> 
> Well, up until now at least, we have handled under the assumption that not
> every user knows exactly what needs to be configured in order to be safe.
> 
> So, we have always aimed for a sane default.
> 
> IOW, if a user wants to disable one mitigation but all 4 are mitigated by the
> same thing, then we probably should issue a warning saying something like:
> 
> 	"If you want to disable W, then you need to disable W, X and Y too in
> 	order to disable W effectively as all 4 are mitigated by the same
> 	mechanism."
> 
> And problem solved.

Makes sense. I'll drop the new parameter and add a warning.

Thanks,
Dan
> 
> IOW, I don't expect someone would consciously want to disable a subset of
> those mitigations but leave the remaining ones on. What usually happens, is
> people do "mitigations=off" in order to regain their performance but not do
> this selective thing which doesn't make a whole lot sense to me anyway.
> 
> Thx.
> 

[PATCH 2/6] x86/bugs: Remove MDS command line

[Daniel Sneddon]

Remove MDS command line option since it can be set using the common
clar_cpu_buffers parameter.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         | 32 -------------------
 arch/x86/kernel/cpu/bugs.c                    | 21 ------------
 2 files changed, 53 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 66b567c4dce5..2753a1e51da5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3231,38 +3231,6 @@
 			Format: <first>,<last>
 			Specifies range of consoles to be captured by the MDA.
 
-	mds=		[X86,INTEL,EARLY]
-			Control mitigation for the Micro-architectural Data
-			Sampling (MDS) vulnerability.
-
-			Certain CPUs are vulnerable to an exploit against CPU
-			internal buffers which can forward information to a
-			disclosure gadget under certain conditions.
-
-			In vulnerable processors, the speculatively
-			forwarded data can be used in a cache side channel
-			attack, to access data to which the attacker does
-			not have direct access.
-
-			This parameter controls the MDS mitigation. The
-			options are:
-
-			full       - Enable MDS mitigation on vulnerable CPUs
-			full,nosmt - Enable MDS mitigation and disable
-				     SMT on vulnerable CPUs
-			off        - Unconditionally disable MDS mitigation
-
-			On TAA-affected machines, mds=off can be prevented by
-			an active TAA mitigation as both vulnerabilities are
-			mitigated with the same mechanism so in order to disable
-			this mitigation, you need to specify tsx_async_abort=off
-			too.
-
-			Not specifying this option is equivalent to
-			mds=full.
-
-			For details see: Documentation/admin-guide/hw-vuln/mds.rst
-
 	mem=nn[KMG]	[HEXAGON,EARLY] Set the memory size.
 			Must be specified, otherwise memory size will be 0.
 
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b3c9e1eede12..ed5524bc3ee4 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -261,27 +261,6 @@ static void __init mds_select_mitigation(void)
 	}
 }
 
-static int __init mds_cmdline(char *str)
-{
-	if (!boot_cpu_has_bug(X86_BUG_MDS))
-		return 0;
-
-	if (!str)
-		return -EINVAL;
-
-	if (!strcmp(str, "off"))
-		mds_mitigation = MDS_MITIGATION_OFF;
-	else if (!strcmp(str, "full"))
-		mds_mitigation = MDS_MITIGATION_FULL;
-	else if (!strcmp(str, "full,nosmt")) {
-		mds_mitigation = MDS_MITIGATION_FULL;
-		mds_nosmt = true;
-	}
-
-	return 0;
-}
-early_param("mds", mds_cmdline);
-
 #undef pr_fmt
 #define pr_fmt(fmt)	"TAA: " fmt
 
-- 
2.25.1

Re: [PATCH 2/6] x86/bugs: Remove MDS command line

[Dave Hansen]

On 9/24/24 15:31, Daniel Sneddon wrote:
> -	mds=		[X86,INTEL,EARLY]
> -			Control mitigation for the Micro-architectural Data
> -			Sampling (MDS) vulnerability.

As much as I'd love to remove old code, won't this surprise users if we
just take this away?

I suspect the _least_ we can do is warn folks if they use something that
we recently ripped out.  But the right thing is probably to keep the old
one around for at least a while. <sigh>

Re: [PATCH 2/6] x86/bugs: Remove MDS command line

[Daniel Sneddon]

On 9/24/24 15:34, Dave Hansen wrote:
> On 9/24/24 15:31, Daniel Sneddon wrote:
>> -	mds=		[X86,INTEL,EARLY]
>> -			Control mitigation for the Micro-architectural Data
>> -			Sampling (MDS) vulnerability.
> 
> As much as I'd love to remove old code, won't this surprise users if we
> just take this away?
> 
> I suspect the _least_ we can do is warn folks if they use something that
> we recently ripped out.  But the right thing is probably to keep the old
> one around for at least a while. <sigh>

True. I can leave the functionality but print a message basically saying "Don't
use this use the new one".

Thanks,
Dan

[PATCH 3/6] x86/bugs: Remove TAA kernel parameter.

[Daniel Sneddon]

Remove tsx_async_abort kernel parameter since it can be set with the common
clar_cpu_buffers parameter.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         | 41 -------------------
 arch/x86/kernel/cpu/bugs.c                    | 21 ----------
 2 files changed, 62 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2753a1e51da5..961e637b8126 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6883,47 +6883,6 @@
 			See Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
 			for more details.
 
-	tsx_async_abort= [X86,INTEL,EARLY] Control mitigation for the TSX Async
-			Abort (TAA) vulnerability.
-
-			Similar to Micro-architectural Data Sampling (MDS)
-			certain CPUs that support Transactional
-			Synchronization Extensions (TSX) are vulnerable to an
-			exploit against CPU internal buffers which can forward
-			information to a disclosure gadget under certain
-			conditions.
-
-			In vulnerable processors, the speculatively forwarded
-			data can be used in a cache side channel attack, to
-			access data to which the attacker does not have direct
-			access.
-
-			This parameter controls the TAA mitigation.  The
-			options are:
-
-			full       - Enable TAA mitigation on vulnerable CPUs
-				     if TSX is enabled.
-
-			full,nosmt - Enable TAA mitigation and disable SMT on
-				     vulnerable CPUs. If TSX is disabled, SMT
-				     is not disabled because CPU is not
-				     vulnerable to cross-thread TAA attacks.
-			off        - Unconditionally disable TAA mitigation
-
-			On MDS-affected machines, tsx_async_abort=off can be
-			prevented by an active MDS mitigation as both vulnerabilities
-			are mitigated with the same mechanism so in order to disable
-			this mitigation, you need to specify mds=off too.
-
-			Not specifying this option is equivalent to
-			tsx_async_abort=full.  On CPUs which are MDS affected
-			and deploy MDS mitigation, TAA mitigation is not
-			required and doesn't provide any additional
-			mitigation.
-
-			For details see:
-			Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
-
 	turbografx.map[2|3]=	[HW,JOY]
 			TurboGraFX parallel port interface
 			Format:
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ed5524bc3ee4..0a09f0d1a343 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -339,27 +339,6 @@ static void __init taa_select_mitigation(void)
 		cpu_smt_disable(false);
 }
 
-static int __init tsx_async_abort_parse_cmdline(char *str)
-{
-	if (!boot_cpu_has_bug(X86_BUG_TAA))
-		return 0;
-
-	if (!str)
-		return -EINVAL;
-
-	if (!strcmp(str, "off")) {
-		taa_mitigation = TAA_MITIGATION_OFF;
-	} else if (!strcmp(str, "full")) {
-		taa_mitigation = TAA_MITIGATION_VERW;
-	} else if (!strcmp(str, "full,nosmt")) {
-		taa_mitigation = TAA_MITIGATION_VERW;
-		taa_nosmt = true;
-	}
-
-	return 0;
-}
-early_param("tsx_async_abort", tsx_async_abort_parse_cmdline);
-
 #undef pr_fmt
 #define pr_fmt(fmt)	"MMIO Stale Data: " fmt
 
-- 
2.25.1

[PATCH 4/6] x86/bugs: Remove MMIO kernel parameter

[Daniel Sneddon]

Remove mmio_stale_data kernel parameter since it can be set with the common
clear_cpu_buffers parameter.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         | 34 -------------------
 arch/x86/kernel/cpu/bugs.c                    | 21 ------------
 2 files changed, 55 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 961e637b8126..7afccd044fb8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3498,40 +3498,6 @@
 			log everything. Information is printed at KERN_DEBUG
 			so loglevel=8 may also need to be specified.
 
-	mmio_stale_data=
-			[X86,INTEL,EARLY] Control mitigation for the Processor
-			MMIO Stale Data vulnerabilities.
-
-			Processor MMIO Stale Data is a class of
-			vulnerabilities that may expose data after an MMIO
-			operation. Exposed data could originate or end in
-			the same CPU buffers as affected by MDS and TAA.
-			Therefore, similar to MDS and TAA, the mitigation
-			is to clear the affected CPU buffers.
-
-			This parameter controls the mitigation. The
-			options are:
-
-			full       - Enable mitigation on vulnerable CPUs
-
-			full,nosmt - Enable mitigation and disable SMT on
-				     vulnerable CPUs.
-
-			off        - Unconditionally disable mitigation
-
-			On MDS or TAA affected machines,
-			mmio_stale_data=off can be prevented by an active
-			MDS or TAA mitigation as these vulnerabilities are
-			mitigated with the same mechanism so in order to
-			disable this mitigation, you need to specify
-			mds=off and tsx_async_abort=off too.
-
-			Not specifying this option is equivalent to
-			mmio_stale_data=full.
-
-			For details see:
-			Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
-
 	<module>.async_probe[=<bool>] [KNL]
 			If no <bool> value is specified or if the value
 			specified is not a valid <bool>, enable asynchronous
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 0a09f0d1a343..63a8cda2fe30 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -414,27 +414,6 @@ static void __init mmio_select_mitigation(void)
 		cpu_smt_disable(false);
 }
 
-static int __init mmio_stale_data_parse_cmdline(char *str)
-{
-	if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA))
-		return 0;
-
-	if (!str)
-		return -EINVAL;
-
-	if (!strcmp(str, "off")) {
-		mmio_mitigation = MMIO_MITIGATION_OFF;
-	} else if (!strcmp(str, "full")) {
-		mmio_mitigation = MMIO_MITIGATION_VERW;
-	} else if (!strcmp(str, "full,nosmt")) {
-		mmio_mitigation = MMIO_MITIGATION_VERW;
-		mmio_nosmt = true;
-	}
-
-	return 0;
-}
-early_param("mmio_stale_data", mmio_stale_data_parse_cmdline);
-
 #undef pr_fmt
 #define pr_fmt(fmt)	"Register File Data Sampling: " fmt
 
-- 
2.25.1

[PATCH 5/6] x86/bugs: Remove RFDS kernel parameter.

[Daniel Sneddon]

Remove the reg_file_data_sampling kernel parameter since it can be set
using the common clear_cpu_buffers parameter.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         | 20 -------------------
 arch/x86/kernel/cpu/bugs.c                    | 17 ----------------
 2 files changed, 37 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7afccd044fb8..2728fef51749 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1182,26 +1182,6 @@
 			The filter can be disabled or changed to another
 			driver later using sysfs.
 
-	reg_file_data_sampling=
-			[X86] Controls mitigation for Register File Data
-			Sampling (RFDS) vulnerability. RFDS is a CPU
-			vulnerability which may allow userspace to infer
-			kernel data values previously stored in floating point
-			registers, vector registers, or integer registers.
-			RFDS only affects Intel Atom processors.
-
-			on:	Turns ON the mitigation.
-			off:	Turns OFF the mitigation.
-
-			This parameter overrides the compile time default set
-			by CONFIG_MITIGATION_RFDS. Mitigation cannot be
-			disabled when other VERW based mitigations (like MDS)
-			are enabled. In order to disable RFDS mitigation all
-			VERW based mitigations need to be disabled.
-
-			For details see:
-			Documentation/admin-guide/hw-vuln/reg-file-data-sampling.rst
-
 	driver_async_probe=  [KNL]
 			List of driver names to be probed asynchronously. *
 			matches with all driver names. If * is specified, the
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 63a8cda2fe30..45411880481c 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -448,23 +448,6 @@ static void __init rfds_select_mitigation(void)
 		rfds_mitigation = RFDS_MITIGATION_UCODE_NEEDED;
 }
 
-static __init int rfds_parse_cmdline(char *str)
-{
-	if (!str)
-		return -EINVAL;
-
-	if (!boot_cpu_has_bug(X86_BUG_RFDS))
-		return 0;
-
-	if (!strcmp(str, "off"))
-		rfds_mitigation = RFDS_MITIGATION_OFF;
-	else if (!strcmp(str, "on"))
-		rfds_mitigation = RFDS_MITIGATION_VERW;
-
-	return 0;
-}
-early_param("reg_file_data_sampling", rfds_parse_cmdline);
-
 #undef pr_fmt
 #define pr_fmt(fmt)     "" fmt
 
-- 
2.25.1

[PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Daniel Sneddon]

The current md_clear routines duplicate a lot of code, and have to be
called twice because if one of the mitigations gets enabled they all
get enabled since it's the same instruction. This approach leads to
code duplication and extra complexity.

The only piece that really changes between the first call of
*_select_mitigation() and the second is X86_FEATURE_CLEAR_CPU_BUF
being set.  Determine if this feature should be set prior to calling
the _select_mitigation() routines. This not only means those functions
only get called once, but it also simplifies them as well.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 191 +++++++++++++++----------------------
 1 file changed, 77 insertions(+), 114 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 45411880481c..412855391184 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -41,7 +41,6 @@ static void __init spectre_v2_user_select_mitigation(void);
 static void __init ssb_select_mitigation(void);
 static void __init l1tf_select_mitigation(void);
 static void __init mds_select_mitigation(void);
-static void __init md_clear_update_mitigation(void);
 static void __init md_clear_select_mitigation(void);
 static void __init taa_select_mitigation(void);
 static void __init mmio_select_mitigation(void);
@@ -244,21 +243,9 @@ static const char * const mds_strings[] = {
 
 static void __init mds_select_mitigation(void)
 {
-	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off()) {
-		mds_mitigation = MDS_MITIGATION_OFF;
-		return;
-	}
-
-	if (mds_mitigation == MDS_MITIGATION_FULL) {
-		if (!boot_cpu_has(X86_FEATURE_MD_CLEAR))
-			mds_mitigation = MDS_MITIGATION_VMWERV;
-
-		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
-
-		if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
-		    (mds_nosmt || cpu_mitigations_auto_nosmt()))
-			cpu_smt_disable(false);
-	}
+	if (mds_mitigation == MDS_MITIGATION_FULL &&
+	    !boot_cpu_has(X86_FEATURE_MD_CLEAR))
+		mds_mitigation = MDS_MITIGATION_VMWERV;
 }
 
 #undef pr_fmt
@@ -284,35 +271,17 @@ static const char * const taa_strings[] = {
 
 static void __init taa_select_mitigation(void)
 {
-	if (!boot_cpu_has_bug(X86_BUG_TAA)) {
-		taa_mitigation = TAA_MITIGATION_OFF;
-		return;
-	}
-
 	/* TSX previously disabled by tsx=off */
 	if (!boot_cpu_has(X86_FEATURE_RTM)) {
 		taa_mitigation = TAA_MITIGATION_TSX_DISABLED;
 		return;
 	}
 
-	if (cpu_mitigations_off()) {
-		taa_mitigation = TAA_MITIGATION_OFF;
+	if (!boot_cpu_has(X86_FEATURE_MD_CLEAR)) {
+		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
 		return;
 	}
 
-	/*
-	 * TAA mitigation via VERW is turned off if both
-	 * tsx_async_abort=off and mds=off are specified.
-	 */
-	if (taa_mitigation == TAA_MITIGATION_OFF &&
-	    mds_mitigation == MDS_MITIGATION_OFF)
-		return;
-
-	if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
-		taa_mitigation = TAA_MITIGATION_VERW;
-	else
-		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
-
 	/*
 	 * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and MDS_NO=1.
 	 * A microcode update fixes this behavior to clear CPU buffers. It also
@@ -325,18 +294,6 @@ static void __init taa_select_mitigation(void)
 	if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
 	    !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
 		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
-
-	/*
-	 * TSX is enabled, select alternate mitigation for TAA which is
-	 * the same as MDS. Enable MDS static branch to clear CPU buffers.
-	 *
-	 * For guests that can't determine whether the correct microcode is
-	 * present on host, enable the mitigation for UCODE_NEEDED as well.
-	 */
-	setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
-
-	if (taa_nosmt || cpu_mitigations_auto_nosmt())
-		cpu_smt_disable(false);
 }
 
 #undef pr_fmt
@@ -360,24 +317,6 @@ static const char * const mmio_strings[] = {
 
 static void __init mmio_select_mitigation(void)
 {
-	if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
-	     boot_cpu_has_bug(X86_BUG_MMIO_UNKNOWN) ||
-	     cpu_mitigations_off()) {
-		mmio_mitigation = MMIO_MITIGATION_OFF;
-		return;
-	}
-
-	if (mmio_mitigation == MMIO_MITIGATION_OFF)
-		return;
-
-	/*
-	 * Enable CPU buffer clear mitigation for host and VMM, if also affected
-	 * by MDS or TAA. Otherwise, enable mitigation for VMM only.
-	 */
-	if (boot_cpu_has_bug(X86_BUG_MDS) || (boot_cpu_has_bug(X86_BUG_TAA) &&
-					      boot_cpu_has(X86_FEATURE_RTM)))
-		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
-
 	/*
 	 * X86_FEATURE_CLEAR_CPU_BUF could be enabled by other VERW based
 	 * mitigations, disable KVM-only mitigation in that case.
@@ -409,9 +348,6 @@ static void __init mmio_select_mitigation(void)
 		mmio_mitigation = MMIO_MITIGATION_VERW;
 	else
 		mmio_mitigation = MMIO_MITIGATION_UCODE_NEEDED;
-
-	if (mmio_nosmt || cpu_mitigations_auto_nosmt())
-		cpu_smt_disable(false);
 }
 
 #undef pr_fmt
@@ -435,16 +371,7 @@ static const char * const rfds_strings[] = {
 
 static void __init rfds_select_mitigation(void)
 {
-	if (!boot_cpu_has_bug(X86_BUG_RFDS) || cpu_mitigations_off()) {
-		rfds_mitigation = RFDS_MITIGATION_OFF;
-		return;
-	}
-	if (rfds_mitigation == RFDS_MITIGATION_OFF)
-		return;
-
-	if (x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR)
-		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
-	else
+	if (!(x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR))
 		rfds_mitigation = RFDS_MITIGATION_UCODE_NEEDED;
 }
 
@@ -485,41 +412,92 @@ static int __init clear_cpu_buffers_cmdline(char *str)
 }
 early_param("clear_cpu_buffers", clear_cpu_buffers_cmdline);
 
-static void __init md_clear_update_mitigation(void)
+static bool __init cpu_bug_needs_verw(void)
 {
-	if (cpu_mitigations_off())
-		return;
+	return boot_cpu_has_bug(X86_BUG_MDS) ||
+	       boot_cpu_has_bug(X86_BUG_TAA) ||
+	       boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
+	       boot_cpu_has_bug(X86_BUG_RFDS);
+}
 
-	if (!boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
-		goto out;
+static bool __init verw_mitigations_disabled(void)
+{
+	/*
+	 * TODO: Create a single mitigation variable that will allow for setting
+	 * the location of the mitigation, i.e.:
+	 *
+	 * kernel->user
+	 * kvm->guest
+	 * kvm->guest if device passthrough
+	 * kernel->idle
+	 */
+	return (mds_mitigation == MDS_MITIGATION_OFF &&
+		taa_mitigation == TAA_MITIGATION_OFF &&
+		mmio_mitigation == MMIO_MITIGATION_OFF &&
+		rfds_mitigation == RFDS_MITIGATION_OFF);
+}
 
+static void __init md_clear_select_mitigation(void)
+{
 	/*
-	 * X86_FEATURE_CLEAR_CPU_BUF is now enabled. Update MDS, TAA and MMIO
-	 * Stale Data mitigation, if necessary.
+	 * If no CPU bug needs VERW, all VERW mitigations are disabled, or all
+	 * mitigations are disabled we bail.
 	 */
-	if (mds_mitigation == MDS_MITIGATION_OFF &&
-	    boot_cpu_has_bug(X86_BUG_MDS)) {
+	if (!cpu_bug_needs_verw() || verw_mitigations_disabled() ||
+	    cpu_mitigations_off()) {
+		mds_mitigation = MDS_MITIGATION_OFF;
+		taa_mitigation = TAA_MITIGATION_OFF;
+		mmio_mitigation = MMIO_MITIGATION_OFF;
+		rfds_mitigation = RFDS_MITIGATION_OFF;
+		goto out;
+	}
+
+	/* Check that at least one mitigation is using the verw mitigaiton.
+	 * If the cpu doesn't have the correct ucode or if the BUG_* is mitigated
+	 * by disabling a feature we won't want to use verw. Ignore MMIO
+	 * for now since it depends on what the others choose.
+	 */
+
+	if (boot_cpu_has_bug(X86_BUG_MDS)) {
 		mds_mitigation = MDS_MITIGATION_FULL;
 		mds_select_mitigation();
+	}  else {
+		mds_mitigation = MDS_MITIGATION_OFF;
 	}
-	if (taa_mitigation == TAA_MITIGATION_OFF &&
-	    boot_cpu_has_bug(X86_BUG_TAA)) {
+	if (boot_cpu_has_bug(X86_BUG_TAA)) {
 		taa_mitigation = TAA_MITIGATION_VERW;
 		taa_select_mitigation();
+	} else {
+		taa_mitigation = TAA_MITIGATION_OFF;
 	}
-	/*
-	 * MMIO_MITIGATION_OFF is not checked here so that mmio_stale_data_clear
-	 * gets updated correctly as per X86_FEATURE_CLEAR_CPU_BUF state.
-	 */
+	if (boot_cpu_has_bug(X86_BUG_RFDS)) {
+		rfds_mitigation = RFDS_MITIGATION_VERW;
+		rfds_select_mitigation();
+	} else {
+		rfds_mitigation = RFDS_MITIGATION_OFF;
+	}
+
+	if (mds_mitigation == MDS_MITIGATION_FULL ||
+	    taa_mitigation == TAA_MITIGATION_VERW ||
+	    rfds_mitigation == RFDS_MITIGATION_VERW)
+		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
+
+	/* Now handle MMIO since it may not use X86_FEATURE_CLEAR_CPU_BUF */
 	if (boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA)) {
 		mmio_mitigation = MMIO_MITIGATION_VERW;
 		mmio_select_mitigation();
+	} else {
+		mmio_mitigation = MMIO_MITIGATION_OFF;
 	}
-	if (rfds_mitigation == RFDS_MITIGATION_OFF &&
-	    boot_cpu_has_bug(X86_BUG_RFDS)) {
-		rfds_mitigation = RFDS_MITIGATION_VERW;
-		rfds_select_mitigation();
-	}
+
+	/* handle nosmt */
+	if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
+	    (mds_nosmt || cpu_mitigations_auto_nosmt()))
+		cpu_smt_disable(false);
+
+	if (taa_nosmt || mmio_nosmt || cpu_mitigations_auto_nosmt())
+		cpu_smt_disable(false);
+
 out:
 	if (boot_cpu_has_bug(X86_BUG_MDS))
 		pr_info("MDS: %s\n", mds_strings[mds_mitigation]);
@@ -533,21 +511,6 @@ static void __init md_clear_update_mitigation(void)
 		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
 }
 
-static void __init md_clear_select_mitigation(void)
-{
-	mds_select_mitigation();
-	taa_select_mitigation();
-	mmio_select_mitigation();
-	rfds_select_mitigation();
-
-	/*
-	 * As these mitigations are inter-related and rely on VERW instruction
-	 * to clear the microarchitural buffers, update and print their status
-	 * after mitigation selection is done for each of these vulnerabilities.
-	 */
-	md_clear_update_mitigation();
-}
-
 #undef pr_fmt
 #define pr_fmt(fmt)	"SRBDS: " fmt
 
-- 
2.25.1

Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Nikolay Borisov]

On 25.09.24 г. 1:31 ч., Daniel Sneddon wrote:
> The current md_clear routines duplicate a lot of code, and have to be
> called twice because if one of the mitigations gets enabled they all
> get enabled since it's the same instruction. This approach leads to
> code duplication and extra complexity.
> 
> The only piece that really changes between the first call of
> *_select_mitigation() and the second is X86_FEATURE_CLEAR_CPU_BUF
> being set.  Determine if this feature should be set prior to calling
> the _select_mitigation() routines. This not only means those functions
> only get called once, but it also simplifies them as well.
> 
> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> ---
>   arch/x86/kernel/cpu/bugs.c | 191 +++++++++++++++----------------------
>   1 file changed, 77 insertions(+), 114 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 45411880481c..412855391184 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -41,7 +41,6 @@ static void __init spectre_v2_user_select_mitigation(void);
>   static void __init ssb_select_mitigation(void);
>   static void __init l1tf_select_mitigation(void);
>   static void __init mds_select_mitigation(void);
> -static void __init md_clear_update_mitigation(void);
>   static void __init md_clear_select_mitigation(void);
>   static void __init taa_select_mitigation(void);
>   static void __init mmio_select_mitigation(void);
> @@ -244,21 +243,9 @@ static const char * const mds_strings[] = {
>   
>   static void __init mds_select_mitigation(void)
>   {
> -	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off()) {
> -		mds_mitigation = MDS_MITIGATION_OFF;
> -		return;
> -	}
> -
> -	if (mds_mitigation == MDS_MITIGATION_FULL) {
> -		if (!boot_cpu_has(X86_FEATURE_MD_CLEAR))
> -			mds_mitigation = MDS_MITIGATION_VMWERV;
> -
> -		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -
> -		if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
> -		    (mds_nosmt || cpu_mitigations_auto_nosmt()))
> -			cpu_smt_disable(false);
> -	}
> +	if (mds_mitigation == MDS_MITIGATION_FULL &&
> +	    !boot_cpu_has(X86_FEATURE_MD_CLEAR))
> +		mds_mitigation = MDS_MITIGATION_VMWERV;
>   }
>   
>   #undef pr_fmt
> @@ -284,35 +271,17 @@ static const char * const taa_strings[] = {
>   
>   static void __init taa_select_mitigation(void)
>   {
> -	if (!boot_cpu_has_bug(X86_BUG_TAA)) {
> -		taa_mitigation = TAA_MITIGATION_OFF;
> -		return;
> -	}
> -
>   	/* TSX previously disabled by tsx=off */
>   	if (!boot_cpu_has(X86_FEATURE_RTM)) {
>   		taa_mitigation = TAA_MITIGATION_TSX_DISABLED;
>   		return;
>   	}
>   
> -	if (cpu_mitigations_off()) {
> -		taa_mitigation = TAA_MITIGATION_OFF;
> +	if (!boot_cpu_has(X86_FEATURE_MD_CLEAR)) {
> +		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
>   		return;
>   	}
>   
> -	/*
> -	 * TAA mitigation via VERW is turned off if both
> -	 * tsx_async_abort=off and mds=off are specified.
> -	 */
> -	if (taa_mitigation == TAA_MITIGATION_OFF &&
> -	    mds_mitigation == MDS_MITIGATION_OFF)
> -		return;
> -
> -	if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> -		taa_mitigation = TAA_MITIGATION_VERW;
> -	else
> -		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> -
>   	/*
>   	 * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and MDS_NO=1.
>   	 * A microcode update fixes this behavior to clear CPU buffers. It also
> @@ -325,18 +294,6 @@ static void __init taa_select_mitigation(void)
>   	if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
>   	    !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
>   		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> -
> -	/*
> -	 * TSX is enabled, select alternate mitigation for TAA which is
> -	 * the same as MDS. Enable MDS static branch to clear CPU buffers.
> -	 *
> -	 * For guests that can't determine whether the correct microcode is
> -	 * present on host, enable the mitigation for UCODE_NEEDED as well.
> -	 */
> -	setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -
> -	if (taa_nosmt || cpu_mitigations_auto_nosmt())
> -		cpu_smt_disable(false);
>   }
>   
>   #undef pr_fmt
> @@ -360,24 +317,6 @@ static const char * const mmio_strings[] = {
>   
>   static void __init mmio_select_mitigation(void)
>   {
> -	if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> -	     boot_cpu_has_bug(X86_BUG_MMIO_UNKNOWN) ||
> -	     cpu_mitigations_off()) {
> -		mmio_mitigation = MMIO_MITIGATION_OFF;
> -		return;
> -	}
> -
> -	if (mmio_mitigation == MMIO_MITIGATION_OFF)
> -		return;
> -
> -	/*
> -	 * Enable CPU buffer clear mitigation for host and VMM, if also affected
> -	 * by MDS or TAA. Otherwise, enable mitigation for VMM only.
> -	 */
> -	if (boot_cpu_has_bug(X86_BUG_MDS) || (boot_cpu_has_bug(X86_BUG_TAA) &&
> -					      boot_cpu_has(X86_FEATURE_RTM)))
> -		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -
>   	/*
>   	 * X86_FEATURE_CLEAR_CPU_BUF could be enabled by other VERW based
>   	 * mitigations, disable KVM-only mitigation in that case.
> @@ -409,9 +348,6 @@ static void __init mmio_select_mitigation(void)
>   		mmio_mitigation = MMIO_MITIGATION_VERW;
>   	else
>   		mmio_mitigation = MMIO_MITIGATION_UCODE_NEEDED;
> -
> -	if (mmio_nosmt || cpu_mitigations_auto_nosmt())
> -		cpu_smt_disable(false);
>   }
>   
>   #undef pr_fmt
> @@ -435,16 +371,7 @@ static const char * const rfds_strings[] = {
>   
>   static void __init rfds_select_mitigation(void)
>   {
> -	if (!boot_cpu_has_bug(X86_BUG_RFDS) || cpu_mitigations_off()) {
> -		rfds_mitigation = RFDS_MITIGATION_OFF;
> -		return;
> -	}
> -	if (rfds_mitigation == RFDS_MITIGATION_OFF)
> -		return;
> -
> -	if (x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR)
> -		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -	else
> +	if (!(x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR))
>   		rfds_mitigation = RFDS_MITIGATION_UCODE_NEEDED;
>   }
>   
> @@ -485,41 +412,92 @@ static int __init clear_cpu_buffers_cmdline(char *str)
>   }
>   early_param("clear_cpu_buffers", clear_cpu_buffers_cmdline);
>   
> -static void __init md_clear_update_mitigation(void)
> +static bool __init cpu_bug_needs_verw(void)
>   {
> -	if (cpu_mitigations_off())
> -		return;
> +	return boot_cpu_has_bug(X86_BUG_MDS) ||
> +	       boot_cpu_has_bug(X86_BUG_TAA) ||
> +	       boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> +	       boot_cpu_has_bug(X86_BUG_RFDS);
> +}
>   
> -	if (!boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
> -		goto out;
> +static bool __init verw_mitigations_disabled(void)
> +{
> +	/*
> +	 * TODO: Create a single mitigation variable that will allow for setting
> +	 * the location of the mitigation, i.e.:
> +	 *
> +	 * kernel->user
> +	 * kvm->guest
> +	 * kvm->guest if device passthrough
> +	 * kernel->idle
> +	 */
> +	return (mds_mitigation == MDS_MITIGATION_OFF &&
> +		taa_mitigation == TAA_MITIGATION_OFF &&
> +		mmio_mitigation == MMIO_MITIGATION_OFF &&
> +		rfds_mitigation == RFDS_MITIGATION_OFF);
> +}
>   
> +static void __init md_clear_select_mitigation(void)
> +{
>   	/*
> -	 * X86_FEATURE_CLEAR_CPU_BUF is now enabled. Update MDS, TAA and MMIO
> -	 * Stale Data mitigation, if necessary.
> +	 * If no CPU bug needs VERW, all VERW mitigations are disabled, or all
> +	 * mitigations are disabled we bail.
>   	 */
> -	if (mds_mitigation == MDS_MITIGATION_OFF &&
> -	    boot_cpu_has_bug(X86_BUG_MDS)) {
> +	if (!cpu_bug_needs_verw() || verw_mitigations_disabled() ||
> +	    cpu_mitigations_off()) {
> +		mds_mitigation = MDS_MITIGATION_OFF;
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +		mmio_mitigation = MMIO_MITIGATION_OFF;
> +		rfds_mitigation = RFDS_MITIGATION_OFF;
> +		goto out;
> +	}
> +
> +	/* Check that at least one mitigation is using the verw mitigaiton.
> +	 * If the cpu doesn't have the correct ucode or if the BUG_* is mitigated
> +	 * by disabling a feature we won't want to use verw. Ignore MMIO
> +	 * for now since it depends on what the others choose.
> +	 */
> +
> +	if (boot_cpu_has_bug(X86_BUG_MDS)) {
>   		mds_mitigation = MDS_MITIGATION_FULL;
>   		mds_select_mitigation();
> +	}  else {
> +		mds_mitigation = MDS_MITIGATION_OFF;
>   	}


BUt with this logic if CONFIG_MITIGATION_MDS is deselected meaning 
mds_mitigations will have the value MDS_MITIGATION_OFF, yet now you will 
set it to _FULL thereby overriding the compile-time value of the user. 
So shouldn't this condition be augmented to alsoo consider 
CONFIG_MITIGATION_MDS compile time value?

<snip>

Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Daniel Sneddon]

On 10/2/24 07:20, Nikolay Borisov wrote:
>> +	if (boot_cpu_has_bug(X86_BUG_MDS)) {
>>   		mds_mitigation = MDS_MITIGATION_FULL;
>>   		mds_select_mitigation();
>> +	}  else {
>> +		mds_mitigation = MDS_MITIGATION_OFF;
>>   	}
> 
> BUt with this logic if CONFIG_MITIGATION_MDS is deselected meaning 
> mds_mitigations will have the value MDS_MITIGATION_OFF, yet now you will 
> set it to _FULL thereby overriding the compile-time value of the user. 
> So shouldn't this condition be augmented to alsoo consider 
> CONFIG_MITIGATION_MDS compile time value?

CONFIG_MITIGATION_MDS is used to set the value of the mds_mitigation variable.
Same goes for all the other mitigations touched here. Those variables are
checked in verw_mitigations_disabled() which is called just before this code. If
all of them are configured off, we return without enabling any of the mitigations.

Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Nikolay Borisov]

On 2.10.24 г. 17:46 ч., Daniel Sneddon wrote:
> On 10/2/24 07:20, Nikolay Borisov wrote:
>>> +	if (boot_cpu_has_bug(X86_BUG_MDS)) {
>>>    		mds_mitigation = MDS_MITIGATION_FULL;
>>>    		mds_select_mitigation();
>>> +	}  else {
>>> +		mds_mitigation = MDS_MITIGATION_OFF;
>>>    	}
>>
>> BUt with this logic if CONFIG_MITIGATION_MDS is deselected meaning
>> mds_mitigations will have the value MDS_MITIGATION_OFF, yet now you will
>> set it to _FULL thereby overriding the compile-time value of the user.
>> So shouldn't this condition be augmented to alsoo consider
>> CONFIG_MITIGATION_MDS compile time value?
> 
> CONFIG_MITIGATION_MDS is used to set the value of the mds_mitigation variable.
> Same goes for all the other mitigations touched here. Those variables are
> checked in verw_mitigations_disabled() which is called just before this code. If
> all of them are configured off, we return without enabling any of the mitigations.

Ah, indeed.

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Josh Poimboeuf]

On Tue, Sep 24, 2024 at 03:31:40PM -0700, Daniel Sneddon wrote:
> +static void __init md_clear_select_mitigation(void)
> +{
>  	/*
> -	 * X86_FEATURE_CLEAR_CPU_BUF is now enabled. Update MDS, TAA and MMIO
> -	 * Stale Data mitigation, if necessary.
> +	 * If no CPU bug needs VERW, all VERW mitigations are disabled, or all
> +	 * mitigations are disabled we bail.
>  	 */

It's already clear what the code is doing, no comment necessary.

> -	if (mds_mitigation == MDS_MITIGATION_OFF &&
> -	    boot_cpu_has_bug(X86_BUG_MDS)) {
> +	if (!cpu_bug_needs_verw() || verw_mitigations_disabled() ||
> +	    cpu_mitigations_off()) {
> +		mds_mitigation = MDS_MITIGATION_OFF;
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +		mmio_mitigation = MMIO_MITIGATION_OFF;
> +		rfds_mitigation = RFDS_MITIGATION_OFF;
> +		goto out;
> +	}

In the case of verw_mitigations_disabled() it's weird to write the
variables again if they're already OFF.  That should be a separate
check.

> +
> +	/* Check that at least one mitigation is using the verw mitigaiton.
> +	 * If the cpu doesn't have the correct ucode or if the BUG_* is mitigated
> +	 * by disabling a feature we won't want to use verw. Ignore MMIO
> +	 * for now since it depends on what the others choose.
> +	 */

Again I think this comment isn't needed as the code is pretty
straightforward.  The only surprise is the MMIO dependency on
X86_FEATURE_CLEAR_CPU_BUF, but that's called out below.

> +
> +	if (boot_cpu_has_bug(X86_BUG_MDS)) {
>  		mds_mitigation = MDS_MITIGATION_FULL;
>  		mds_select_mitigation();
> +	}  else {
> +		mds_mitigation = MDS_MITIGATION_OFF;
>  	}
> -	if (taa_mitigation == TAA_MITIGATION_OFF &&
> -	    boot_cpu_has_bug(X86_BUG_TAA)) {
> +	if (boot_cpu_has_bug(X86_BUG_TAA)) {
>  		taa_mitigation = TAA_MITIGATION_VERW;
>  		taa_select_mitigation();
> +	} else {
> +		taa_mitigation = TAA_MITIGATION_OFF;
>  	}
> -	/*
> -	 * MMIO_MITIGATION_OFF is not checked here so that mmio_stale_data_clear
> -	 * gets updated correctly as per X86_FEATURE_CLEAR_CPU_BUF state.
> -	 */
> +	if (boot_cpu_has_bug(X86_BUG_RFDS)) {
> +		rfds_mitigation = RFDS_MITIGATION_VERW;
> +		rfds_select_mitigation();
> +	} else {
> +		rfds_mitigation = RFDS_MITIGATION_OFF;
> +	}

This spaghetti can be simplifed by relying on *_select_mitigation() to
set the mitigation, for example:

static void __init mds_select_mitigation(void)
{
	if (!boot_cpu_has_bug(X86_BUG_MDS))
		mds_mitigation = MDS_MITIGATION_OFF;
	else if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
		mds_mitigation = MDS_MITIGATION_VERW;
	else
		mds_mitigation = MDS_MITIGATION_VMWERV;
}

Then you can just do:

	mds_select_mitigation();
	taa_select_mitigation();
	rfds_select_mitigation();


> +	if (mds_mitigation == MDS_MITIGATION_FULL ||
> +	    taa_mitigation == TAA_MITIGATION_VERW ||
> +	    rfds_mitigation == RFDS_MITIGATION_VERW)

For consistency can we rename MDS_MITIGATION_FULL to
MDS_MITIGATION_VERW?

> +		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> +
> +	/* Now handle MMIO since it may not use X86_FEATURE_CLEAR_CPU_BUF */

I would clarify this a bit, something like:

	/*
	 * The MMIO mitigation has a dependency on
	 * X86_FEATURE_CLEAR_CPU_BUF so this must be called after it
	 * gets set.
	 */

>  	if (boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA)) {
>  		mmio_mitigation = MMIO_MITIGATION_VERW;
>  		mmio_select_mitigation();
> +	} else {
> +		mmio_mitigation = MMIO_MITIGATION_OFF;
>  	}
> -	if (rfds_mitigation == RFDS_MITIGATION_OFF &&
> -	    boot_cpu_has_bug(X86_BUG_RFDS)) {
> -		rfds_mitigation = RFDS_MITIGATION_VERW;
> -		rfds_select_mitigation();
> -	}
> +
> +	/* handle nosmt */

Again I think this comment is superfluous.

> +	if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
> +	    (mds_nosmt || cpu_mitigations_auto_nosmt()))
> +		cpu_smt_disable(false);
> +
> +	if (taa_nosmt || mmio_nosmt || cpu_mitigations_auto_nosmt())
> +		cpu_smt_disable(false);
> +

-- 
Josh

Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations

[Daniel Sneddon]

On 10/7/24 12:37, Josh Poimboeuf wrote:
> On Tue, Sep 24, 2024 at 03:31:40PM -0700, Daniel Sneddon wrote:
>> +static void __init md_clear_select_mitigation(void)
>> +{
>>  	/*
>> -	 * X86_FEATURE_CLEAR_CPU_BUF is now enabled. Update MDS, TAA and MMIO
>> -	 * Stale Data mitigation, if necessary.
>> +	 * If no CPU bug needs VERW, all VERW mitigations are disabled, or all
>> +	 * mitigations are disabled we bail.
>>  	 */
> 
> It's already clear what the code is doing, no comment necessary.
> 
Will remove.
>> -	if (mds_mitigation == MDS_MITIGATION_OFF &&
>> -	    boot_cpu_has_bug(X86_BUG_MDS)) {
>> +	if (!cpu_bug_needs_verw() || verw_mitigations_disabled() ||
>> +	    cpu_mitigations_off()) {
>> +		mds_mitigation = MDS_MITIGATION_OFF;
>> +		taa_mitigation = TAA_MITIGATION_OFF;
>> +		mmio_mitigation = MMIO_MITIGATION_OFF;
>> +		rfds_mitigation = RFDS_MITIGATION_OFF;
>> +		goto out;
>> +	}
> 
> In the case of verw_mitigations_disabled() it's weird to write the
> variables again if they're already OFF.  That should be a separate
> check.
> 
Sure. I will separate them out.
>> +
>> +	/* Check that at least one mitigation is using the verw mitigaiton.
>> +	 * If the cpu doesn't have the correct ucode or if the BUG_* is mitigated
>> +	 * by disabling a feature we won't want to use verw. Ignore MMIO
>> +	 * for now since it depends on what the others choose.
>> +	 */
> 
> Again I think this comment isn't needed as the code is pretty
> straightforward.  The only surprise is the MMIO dependency on
> X86_FEATURE_CLEAR_CPU_BUF, but that's called out below.
> 
Will remove.
>> +
>> +	if (boot_cpu_has_bug(X86_BUG_MDS)) {
>>  		mds_mitigation = MDS_MITIGATION_FULL;
>>  		mds_select_mitigation();
>> +	}  else {
>> +		mds_mitigation = MDS_MITIGATION_OFF;
>>  	}
>> -	if (taa_mitigation == TAA_MITIGATION_OFF &&
>> -	    boot_cpu_has_bug(X86_BUG_TAA)) {
>> +	if (boot_cpu_has_bug(X86_BUG_TAA)) {
>>  		taa_mitigation = TAA_MITIGATION_VERW;
>>  		taa_select_mitigation();
>> +	} else {
>> +		taa_mitigation = TAA_MITIGATION_OFF;
>>  	}
>> -	/*
>> -	 * MMIO_MITIGATION_OFF is not checked here so that mmio_stale_data_clear
>> -	 * gets updated correctly as per X86_FEATURE_CLEAR_CPU_BUF state.
>> -	 */
>> +	if (boot_cpu_has_bug(X86_BUG_RFDS)) {
>> +		rfds_mitigation = RFDS_MITIGATION_VERW;
>> +		rfds_select_mitigation();
>> +	} else {
>> +		rfds_mitigation = RFDS_MITIGATION_OFF;
>> +	}
> 
> This spaghetti can be simplifed by relying on *_select_mitigation() to
> set the mitigation, for example:
> 
> static void __init mds_select_mitigation(void)
> {
> 	if (!boot_cpu_has_bug(X86_BUG_MDS))
> 		mds_mitigation = MDS_MITIGATION_OFF;
> 	else if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> 		mds_mitigation = MDS_MITIGATION_VERW;
> 	else
> 		mds_mitigation = MDS_MITIGATION_VMWERV;
> }
> 
> Then you can just do:
> 
> 	mds_select_mitigation();
> 	taa_select_mitigation();
> 	rfds_select_mitigation();
> 
> 
You're right. That is much cleaner. Will fix.
>> +	if (mds_mitigation == MDS_MITIGATION_FULL ||
>> +	    taa_mitigation == TAA_MITIGATION_VERW ||
>> +	    rfds_mitigation == RFDS_MITIGATION_VERW)
> 
> For consistency can we rename MDS_MITIGATION_FULL to
> MDS_MITIGATION_VERW?
> 
Will do!
>> +		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
>> +
>> +	/* Now handle MMIO since it may not use X86_FEATURE_CLEAR_CPU_BUF */
> 
> I would clarify this a bit, something like:
> 
> 	/*
> 	 * The MMIO mitigation has a dependency on
> 	 * X86_FEATURE_CLEAR_CPU_BUF so this must be called after it
> 	 * gets set.
> 	 */
> 
Will update.
>>  	if (boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA)) {
>>  		mmio_mitigation = MMIO_MITIGATION_VERW;
>>  		mmio_select_mitigation();
>> +	} else {
>> +		mmio_mitigation = MMIO_MITIGATION_OFF;
>>  	}
>> -	if (rfds_mitigation == RFDS_MITIGATION_OFF &&
>> -	    boot_cpu_has_bug(X86_BUG_RFDS)) {
>> -		rfds_mitigation = RFDS_MITIGATION_VERW;
>> -		rfds_select_mitigation();
>> -	}
>> +
>> +	/* handle nosmt */
> 
> Again I think this comment is superfluous.
> 
Will remove.
>> +	if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
>> +	    (mds_nosmt || cpu_mitigations_auto_nosmt()))
>> +		cpu_smt_disable(false);
>> +
>> +	if (taa_nosmt || mmio_nosmt || cpu_mitigations_auto_nosmt())
>> +		cpu_smt_disable(false);
>> +
> 

Thanks for the review!