2023-10-10 10:31:25

by Breno Leitao

[permalink] [raw]
Subject: [PATCH v4] x86/bugs: Add a separate config for each mitigation

Create an entry for each CPU mitigation under
CONFIG_SPECULATION_MITIGATIONS. This allow users to enable or disable
them at compilation time.

Currently, the CONFIG_SPECULATION_MITIGATIONS is halfway populated,
where some mitigations have entries in Kconfig, and they could be
modified, while others mitigations do not have Kconfig entries, and
could not be controlled at build time.

The fact of having a fine grained control can help in a few ways:

1) Users can choose and pick only mitigations that are important for
their workloads.

2) Users and developers can choose to disable mitigations that mangle
the assembly code generation, making it hard to read.

3) Separate configs for just source code readability, so that developers
see *which* butt-ugly piece of crap code is for what reason.

Important to say, if a mitigation is disabled at compilation time, it
could be enabled at runtime using kernel command line arguments.

Discussion about this approach:
https://lore.kernel.org/all/CAHk-=wjTHeQjsqtHcBGvy9TaJQ5uAm5HrCDuOD9v7qA9U1Xr4w@mail.gmail.com/

Signed-off-by: Breno Leitao <[email protected]>

---
V1:
* Creates a way to mitigate all (or none) hardware bugs
V2:
* Create KCONFIGs entries only some hardware bugs (MDS, TAA, MMIO)
V3:
* Expand the mitigations KCONFIGs to all hardware bugs that are
Linux mitigates.
V4:
* Patch rebase
* Better documentation about the reasons of this decision
---
arch/x86/Kconfig | 93 ++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/bugs.c | 50 +++++++++++++++++---
2 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d4a73d12780a..9eeeed4ba0bb 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2585,6 +2585,99 @@ config GDS_FORCE_MITIGATION

If in doubt, say N.

+config MITIGATE_MDS
+ bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"
+ depends on CPU_SUP_INTEL
+ default y
+ help
+ Enable mitigation for Microarchitectural Data Sampling (MDS). MDS is
+ a hardware vulnerability which allows unprivileged speculative access
+ to data which is available in various CPU internal buffer. Deeper
+ technical information is available in the MDS specific x86 architecture
+ section: Documentation/arch/x86/mds.rst.
+
+config MITIGATE_TAA
+ bool "Mitigate TSX Asynchronous Abort (TAA) hardware bug"
+ depends on CPU_SUP_INTEL
+ default y
+ help
+ Enable mitigation for TSX Asynchronous Abort (TAA). TAA is a hardware
+ vulnerability that allows unprivileged speculative access to data
+ which is available in various CPU internal buffers by using
+ asynchronous aborts within an Intel TSX transactional region.
+
+config MITIGATE_MMIO_STALE_DATA
+ bool "Mitigate MMIO Stale Data hardware bug"
+ depends on CPU_SUP_INTEL
+ default y
+ help
+ Enable mitigation for MMIO Stale Data hardware bugs. Processor MMIO
+ Stale Data Vulnerabilities are a class of memory-mapped I/O (MMIO)
+ vulnerabilities that can expose data. The vulnerabilities require the
+ attacker to have access to MMIO.
+
+config MITIGATE_L1TF
+ bool "Mitigate L1 Terminal Fault (L1TF) hardware bug"
+ default y
+ help
+ Mitigate L1 Terminal Fault (L1TF) hardware bug. L1 Terminal Fault is a
+ hardware vulnerability which allows unprivileged speculative access to data
+ which is available in the Level 1 Data Cache when the page table
+ entry controlling the virtual address.
+
+config MITIGATE_RETBLEED
+ bool "Mitigate RETBleed hardware bug"
+ default y
+ help
+ Enable mitigation for RETBleed (Arbitrary Speculative Code Execution
+ with Return Instructions) vulnerability. RETBleed is a speculative
+ execution attack which takes advantage of microarchitectural behavior
+ in many modern microprocessors, similar to Spectre v2. An
+ unprivileged attacker can use these flaws to bypass conventional
+ memory security restrictions to gain read access to privileged memory
+ that would otherwise be inaccessible.
+
+config MITIGATE_SPECTRE_V1
+ bool "Mitigate SPECTRE V1 hardware bug"
+ default y
+ help
+ Enable mitigation for Spectre V1 (Bounds Check Bypass). Spectre V1 is a
+ class of side channel attacks that takes advantage of speculative
+ execution that bypasses conditional branch instructions used for
+ memory access bounds check.
+
+config MITIGATE_SPECTRE_V2
+ bool "Mitigate SPECTRE V2 hardware bug"
+ default y
+ help
+ Enable mitigation for Spectre V2 (Branch Target Injection). Spectre
+ V2 is a class of side channel attacks that takes advantage of
+ indirect branch predictors inside the processor. In Spectre variant 2
+ attacks, the attacker can steer speculative indirect branches in the
+ victim to gadget code by poisoning the branch target buffer of a CPU
+ used for predicting indirect branch addresses.
+
+config MITIGATE_SRBDS
+ bool "Mitigate Special Register Buffer Data Sampling (SRBDS) hardware bug"
+ depends on CPU_SUP_INTEL
+ default y
+ help
+ Enable mitigation for Special Register Buffer Data Sampling (SRBDS).
+ SRBDS is a hardware vulnerability that allows Microarchitectural Data
+ Sampling (MDS) techniques to infer values returned from special
+ register accesses. An unprivileged user can extract values returned
+ from RDRAND and RDSEED executed on another core or sibling thread
+ using MDS techniques.
+
+config MITIGATE_SSB
+ bool "Mitigate Speculative Store Bypass (SSB) hardware bug"
+ default y
+ help
+ Enable mitigation for Speculative Store Bypass (SSB). SSB is a
+ hardware security vulnerability and its exploitation takes advantage
+ of speculative execution in a similar way to the Meltdown and Spectre
+ security vulnerabilities.
+
endif

config ARCH_HAS_ADD_PAGES
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bb0ab8466b91..64aa77447e4b 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -232,7 +232,11 @@ static void x86_amd_ssb_disable(void)
#define pr_fmt(fmt) "MDS: " fmt

/* Default mitigation for MDS-affected CPUs */
+#if IS_ENABLED(CONFIG_MITIGATE_MDS)
static enum mds_mitigations mds_mitigation __ro_after_init = MDS_MITIGATION_FULL;
+#else
+static enum mds_mitigations mds_mitigation __ro_after_init = MDS_MITIGATION_OFF;
+#endif
static bool mds_nosmt __ro_after_init = false;

static const char * const mds_strings[] = {
@@ -292,7 +296,11 @@ enum taa_mitigations {
};

/* Default mitigation for TAA-affected CPUs */
+#if IS_ENABLED(CONFIG_MITIGATE_TAA)
static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
+#else
+static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_OFF;
+#endif
static bool taa_nosmt __ro_after_init;

static const char * const taa_strings[] = {
@@ -393,7 +401,11 @@ enum mmio_mitigations {
};

/* Default mitigation for Processor MMIO Stale Data vulnerabilities */
+#if IS_ENABLED(CONFIG_MITIGATE_MMIO_STALE_DATA)
static enum mmio_mitigations mmio_mitigation __ro_after_init = MMIO_MITIGATION_VERW;
+#else
+static enum mmio_mitigations mmio_mitigation __ro_after_init = MMIO_MITIGATION_OFF;
+#endif
static bool mmio_nosmt __ro_after_init = false;

static const char * const mmio_strings[] = {
@@ -542,7 +554,11 @@ enum srbds_mitigations {
SRBDS_MITIGATION_HYPERVISOR,
};

+#if IS_ENABLED(CONFIG_MITIGATE_SRBDS)
static enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL;
+#else
+static enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_OFF;
+#endif

static const char * const srbds_strings[] = {
[SRBDS_MITIGATION_OFF] = "Vulnerable",
@@ -810,8 +826,13 @@ enum spectre_v1_mitigation {
SPECTRE_V1_MITIGATION_AUTO,
};

+#if IS_ENABLED(CONFIG_MITIGATE_SPECTRE_V1)
static enum spectre_v1_mitigation spectre_v1_mitigation __ro_after_init =
SPECTRE_V1_MITIGATION_AUTO;
+#else
+static enum spectre_v1_mitigation spectre_v1_mitigation __ro_after_init =
+ SPECTRE_V1_MITIGATION_NONE;
+#endif

static const char * const spectre_v1_strings[] = {
[SPECTRE_V1_MITIGATION_NONE] = "Vulnerable: __user pointer sanitization and usercopy barriers only; no swapgs barriers",
@@ -925,8 +946,13 @@ static const char * const retbleed_strings[] = {

static enum retbleed_mitigation retbleed_mitigation __ro_after_init =
RETBLEED_MITIGATION_NONE;
+#if IS_ENABLED(CONFIG_MITIGATE_RETBLEED)
static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
RETBLEED_CMD_AUTO;
+#else
+static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
+ RETBLEED_CMD_OFF;
+#endif

static int __ro_after_init retbleed_nosmt = false;

@@ -1386,7 +1412,11 @@ static void __init spec_v2_print_cond(const char *reason, bool secure)

static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
{
+#if IS_ENABLED(CONFIG_MITIGATE_SPECTRE_V2)
enum spectre_v2_mitigation_cmd cmd = SPECTRE_V2_CMD_AUTO;
+#else
+ enum spectre_v2_mitigation_cmd cmd = SPECTRE_V2_CMD_NONE;
+#endif
char arg[20];
int ret, i;

@@ -1396,7 +1426,7 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)

ret = cmdline_find_option(boot_command_line, "spectre_v2", arg, sizeof(arg));
if (ret < 0)
- return SPECTRE_V2_CMD_AUTO;
+ return cmd;

for (i = 0; i < ARRAY_SIZE(mitigation_options); i++) {
if (!match_option(arg, ret, mitigation_options[i].option))
@@ -1406,8 +1436,8 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
}

if (i >= ARRAY_SIZE(mitigation_options)) {
- pr_err("unknown option (%s). Switching to AUTO select\n", arg);
- return SPECTRE_V2_CMD_AUTO;
+ pr_err("unknown option (%s). Switching to default mode\n", arg);
+ return cmd;
}

if ((cmd == SPECTRE_V2_CMD_RETPOLINE ||
@@ -1880,7 +1910,11 @@ static const struct {

static enum ssb_mitigation_cmd __init ssb_parse_cmdline(void)
{
+#if IS_ENABLED(CONFIG_MITIGATE_SSB)
enum ssb_mitigation_cmd cmd = SPEC_STORE_BYPASS_CMD_AUTO;
+#else
+ enum ssb_mitigation_cmd cmd = SPEC_STORE_BYPASS_CMD_NONE;
+#endif
char arg[20];
int ret, i;

@@ -1891,7 +1925,7 @@ static enum ssb_mitigation_cmd __init ssb_parse_cmdline(void)
ret = cmdline_find_option(boot_command_line, "spec_store_bypass_disable",
arg, sizeof(arg));
if (ret < 0)
- return SPEC_STORE_BYPASS_CMD_AUTO;
+ return cmd;

for (i = 0; i < ARRAY_SIZE(ssb_mitigation_options); i++) {
if (!match_option(arg, ret, ssb_mitigation_options[i].option))
@@ -1902,8 +1936,8 @@ static enum ssb_mitigation_cmd __init ssb_parse_cmdline(void)
}

if (i >= ARRAY_SIZE(ssb_mitigation_options)) {
- pr_err("unknown option (%s). Switching to AUTO select\n", arg);
- return SPEC_STORE_BYPASS_CMD_AUTO;
+ pr_err("unknown option (%s). Switching to default mode\n", arg);
+ return cmd;
}
}

@@ -2230,7 +2264,11 @@ EXPORT_SYMBOL_GPL(itlb_multihit_kvm_mitigation);
#define pr_fmt(fmt) "L1TF: " fmt

/* Default mitigation for L1TF-affected CPUs */
+#if IS_ENABLED(CONFIG_MITIGATE_L1TF)
enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH;
+#else
+enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_OFF;
+#endif
#if IS_ENABLED(CONFIG_KVM_INTEL)
EXPORT_SYMBOL_GPL(l1tf_mitigation);
#endif
--
2.34.1


2023-10-11 04:43:01

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Tue, Oct 10, 2023 at 03:30:27AM -0700, Breno Leitao wrote:
> +config MITIGATE_MDS
> + bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"
> + depends on CPU_SUP_INTEL
> + default y
> + help
> + Enable mitigation for Microarchitectural Data Sampling (MDS). MDS is
> + a hardware vulnerability which allows unprivileged speculative access
> + to data which is available in various CPU internal buffer. Deeper
> + technical information is available in the MDS specific x86 architecture
> + section: Documentation/arch/x86/mds.rst.

MITIGATE_GDS seems to be missing?

For consistency I'm thinking it would be better to call them
MITIGATION_* rather than MITIGATE_* since they're under the
SPECULATION_MITIGATIONS menuentry, and the bugs code almost exclusively
uses the noun "mitigation" rather than the verb "mitigate".

While we're at it should we go ahead and rename the existing options?

e.g.:

MITIGATION_PAGE_TABLE_ISOLATION
MITIGATION_RETPOLINE
MITIGATION_UNRET_ENTRY
MITIGATION_CALL_DEPTH_TRACKING
MITIGATION_IBPB_ENTRY
MITIGATION_IBRS_ENTRY
MITIGATION_SRSO
MITIGATION_SLS

--
Josh

2023-10-11 12:24:33

by Breno Leitao

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Tue, Oct 10, 2023 at 09:42:52PM -0700, Josh Poimboeuf wrote:
> On Tue, Oct 10, 2023 at 03:30:27AM -0700, Breno Leitao wrote:
> > +config MITIGATE_MDS
> > + bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"
> > + depends on CPU_SUP_INTEL
> > + default y
> > + help
> > + Enable mitigation for Microarchitectural Data Sampling (MDS). MDS is
> > + a hardware vulnerability which allows unprivileged speculative access
> > + to data which is available in various CPU internal buffer. Deeper
> > + technical information is available in the MDS specific x86 architecture
> > + section: Documentation/arch/x86/mds.rst.
>
> MITIGATE_GDS seems to be missing?

GDS is named `GDS_FORCE_MITIGATION` instead of MITIGATE_GDS. See
11cf22869ea ("x86/speculation: Add Kconfig option for GDS")
>
> For consistency I'm thinking it would be better to call them
> MITIGATION_* rather than MITIGATE_* since they're under the
> SPECULATION_MITIGATIONS menuentry, and the bugs code almost exclusively
> uses the noun "mitigation" rather than the verb "mitigate".

Sure. I can definitely rename them to MITIGATION_. Let's see if no one
else disagree and I will update the patch.


> While we're at it should we go ahead and rename the existing options?

Sure!

Thanks for the feedback!

2023-10-11 16:50:31

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Wed, Oct 11, 2023 at 05:20:59AM -0700, Breno Leitao wrote:
> On Tue, Oct 10, 2023 at 09:42:52PM -0700, Josh Poimboeuf wrote:
> > On Tue, Oct 10, 2023 at 03:30:27AM -0700, Breno Leitao wrote:
> > > +config MITIGATE_MDS
> > > + bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"
> > > + depends on CPU_SUP_INTEL
> > > + default y
> > > + help
> > > + Enable mitigation for Microarchitectural Data Sampling (MDS). MDS is
> > > + a hardware vulnerability which allows unprivileged speculative access
> > > + to data which is available in various CPU internal buffer. Deeper
> > > + technical information is available in the MDS specific x86 architecture
> > > + section: Documentation/arch/x86/mds.rst.
> >
> > MITIGATE_GDS seems to be missing?
>
> GDS is named `GDS_FORCE_MITIGATION` instead of MITIGATE_GDS. See
> 11cf22869ea ("x86/speculation: Add Kconfig option for GDS")

Enabling/disabling GDS_FORCE_MITIGATION doesn't enable/disable the
mitigation, it just changes the default.

For consistency that option should probably be renamed to
MITIGATION_GDS_FORCE. It should depend on MITIGATION_GDS. If
MITIGATION_GDS is disabled, gds_mitigation should default to
GDS_MITIGATION_OFF.

e.g., something like

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 66bfabae8814..c8c879302b37 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2549,9 +2549,16 @@ config SLS
against straight line speculation. The kernel image might be slightly
larger.

-config GDS_FORCE_MITIGATION
- bool "Force GDS Mitigation"
+config MITIGATION_GDS
+ bool" Mitigate Gather Data Sampling"
depends on CPU_SUP_INTEL
+ default y
+ help
+ (Insert actual description here)
+
+config MITIGATION_GDS_FORCE
+ bool "Force GDS Mitigation"
+ depends on MITIGATION_GDS
default n
help
Gather Data Sampling (GDS) is a hardware vulnerability which allows
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index f081d26616ac..43ad3eee3d4c 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -671,11 +671,15 @@ enum gds_mitigations {
GDS_MITIGATION_HYPERVISOR,
};

-#if IS_ENABLED(CONFIG_GDS_FORCE_MITIGATION)
+#if IS_ENABLED(CONFIG_MITIGATION_GDS)
+#if IS_ENABLED(CONFIG_MITIGATION_GDS_FORCE)
static enum gds_mitigations gds_mitigation __ro_after_init = GDS_MITIGATION_FORCE;
#else
static enum gds_mitigations gds_mitigation __ro_after_init = GDS_MITIGATION_FULL;
#endif
+#else
+static enum gds_mitigations gds_mitigation __ro_after_init = GDS_MITIGATION_OFF;
+#endif

static const char * const gds_strings[] = {
[GDS_MITIGATION_OFF] = "Vulnerable",

2023-10-11 19:03:48

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Tue, Oct 10, 2023 at 09:42:52PM -0700, Josh Poimboeuf wrote:
> MITIGATION_PAGE_TABLE_ISOLATION
> MITIGATION_RETPOLINE
> MITIGATION_UNRET_ENTRY
> MITIGATION_CALL_DEPTH_TRACKING
> MITIGATION_IBPB_ENTRY
> MITIGATION_IBRS_ENTRY
> MITIGATION_SRSO
> MITIGATION_SLS

The train has already left the station on those. The other mitigations
don't have "MITIGAT*" at all in front of the name. I.e.:

config RETHUNK
config CALL_DEPTH_TRACKING
...

and prepending them all with MITIGATION_ is going to cause too much
senseless churn for a reason which I don't think is worth the effort.

So let's stick with the current naming scheme and do

config MDS
config TAA
...

and so on.

Thx.

--
Regards/Gruss,
Boris.

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

2023-10-11 22:01:28

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation


* Breno Leitao <[email protected]> wrote:

> +config MITIGATE_MDS
> + bool "Mitigate Microarchitectural Data Sampling (MDS) hardware bug"

> +config MITIGATE_TAA
> + bool "Mitigate TSX Asynchronous Abort (TAA) hardware bug"

> +config MITIGATE_MMIO_STALE_DATA
> + bool "Mitigate MMIO Stale Data hardware bug"

> +config MITIGATE_L1TF
> + bool "Mitigate L1 Terminal Fault (L1TF) hardware bug"

> +config MITIGATE_RETBLEED
> + bool "Mitigate RETBleed hardware bug"

> +config MITIGATE_SPECTRE_V1
> + bool "Mitigate SPECTRE V1 hardware bug"

> +config MITIGATE_SPECTRE_V2
> + bool "Mitigate SPECTRE V2 hardware bug"

> +config MITIGATE_SRBDS
> + bool "Mitigate Special Register Buffer Data Sampling (SRBDS) hardware bug"

> +config MITIGATE_SSB
> + bool "Mitigate Speculative Store Bypass (SSB) hardware bug"

> +#if IS_ENABLED(CONFIG_MITIGATE_RETBLEED)
> static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> RETBLEED_CMD_AUTO;
> +#else
> +static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> + RETBLEED_CMD_OFF;
> +#endif

1)

Yeah, so this #ifdeffery is unnecessarily ugly - we can actually assign
integer values in the Kconfig language and use that for initialization.

Is there a reason why we wouldn't want to do something like:

static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED;

... or so?

2)

The new Kconfig namespace should probably be X86_MITIGATE_*, so that we
don't crowd the generic kernel's Kconfig namespace.

3)

And yes, now that the rush of CPU vulnerabilities seems to be ebbing, we
should probably consider unifying the existing hodgepodge of mitigation
Kconfig options as well, to not build up even more technical debt.

The churn factor seems moderate:

kepler:~/tip> git grep CONFIG_RETPOLINE | wc -l
52

kepler:~/tip> git grep RETHUNK | wc -l
42

kepler:~/tip> git grep CALL_DEPTH_TRACKING | wc -l
24

... and since most of this code is maintained in the same tree, the usual
arguments against churn (interfering with other Git trees) does not apply
nearly as much.

4)

Fourth, I think we should inform users (in the boot log) when a kernel
.config changes a mitigation default value compared from what the upstream
kernel thinks is a suitable default.

Sometimes it can be a simple configuration mistake, or a user might have
different opinion about the importance of a particular mitigation. Nothing
heavy-handed, just a simple pr_info() table of changes?

Thanks,

Ingo

2023-10-11 22:03:43

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Wed, Oct 11, 2023 at 09:03:17PM +0200, Borislav Petkov wrote:
> On Tue, Oct 10, 2023 at 09:42:52PM -0700, Josh Poimboeuf wrote:
> > MITIGATION_PAGE_TABLE_ISOLATION
> > MITIGATION_RETPOLINE
> > MITIGATION_UNRET_ENTRY
> > MITIGATION_CALL_DEPTH_TRACKING
> > MITIGATION_IBPB_ENTRY
> > MITIGATION_IBRS_ENTRY
> > MITIGATION_SRSO
> > MITIGATION_SLS
>
> The train has already left the station on those. The other mitigations
> don't have "MITIGAT*" at all in front of the name. I.e.:
>
> config RETHUNK
> config CALL_DEPTH_TRACKING
> ...
>
> and prepending them all with MITIGATION_ is going to cause too much
> senseless churn for a reason which I don't think is worth the effort.

I wouldn't call it senseless churn. There are concrete benefits -- for
both users and developers -- of having all the mitigation config options
living in the same config namespace.

Sure, the change might cause pain for users who disable these options,
but do those users actually exist?

--
Josh

2023-10-12 07:30:07

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Wed, Oct 11, 2023 at 03:03:25PM -0700, Josh Poimboeuf wrote:
> I wouldn't call it senseless churn. There are concrete benefits -- for
> both users and developers -- of having all the mitigation config options
> living in the same config namespace.

I don't see it. What does the same namespace give you? So you see in the
code a bunch of ifdeffery and some or all of them have
CONFIG_MITIGATION_ prepended. To me it doesn't matter whether they're
mitigations or not - it is just the next Kconfig symbol.

> Sure, the change might cause pain for users who disable these options,
> but do those users actually exist?

Apparently there are use cases where people simply want to disable all
that mitigation crap because they want their performance back. This
thread being one. And I know of another one with a similar sentiment.

--
Regards/Gruss,
Boris.

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

2023-10-12 13:03:00

by Breno Leitao

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

Hello Ingo,

On Wed, Oct 11, 2023 at 11:59:47PM +0200, Ingo Molnar wrote:
>
> * Breno Leitao <[email protected]> wrote:

> > +#if IS_ENABLED(CONFIG_MITIGATE_RETBLEED)
> > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > RETBLEED_CMD_AUTO;
> > +#else
> > +static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > + RETBLEED_CMD_OFF;
> > +#endif
>
> 1)
>
> Yeah, so this #ifdeffery is unnecessarily ugly - we can actually assign
> integer values in the Kconfig language and use that for initialization.
>
> Is there a reason why we wouldn't want to do something like:
>
> static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED;
>
> ... or so?

Yes. There are two reasons rigth now:

1) How to avoid the "undefined" behaviour when
CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED is not defined ? Something as:

error: ‘CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED’ undeclared (first use in this function)

2) Right now, these _cmd values are all different by default. Here are a few
examples when the kernel is compiled with the mitigations:

retbleed_cmd = RETBLEED_CMD_AUTO (1)
spectre_v2_mitigation_cmd = SPECTRE_V2_CMD_AUTO (1)
ssb_mitigation_cmd = SPEC_STORE_BYPASS_CMD_AUTO (1)
l1tf_mitigation = L1TF_MITIGATION_FLUSH(2)
mds_mitigation = MDS_MITIGATION_FULL(1)
taa_mitigation = TAA_MITIGATION_VERW (2)
mmio_mitigation = MMIO_MITIGATION_VERW (2)
gds_mitigation = GDS_MITIGATION_FULL (3)

If there is a solution for 1, then I _think_ we can probably reorder the
enums, so, the "AUTO" value is always 1?!


> 2)
>
> The new Kconfig namespace should probably be X86_MITIGATE_*, so that we
> don't crowd the generic kernel's Kconfig namespace.

There is another sub-thread under this patchset that is discussing
naming as well. Boris wants to keep it as simple as it is today:

https://lore.kernel.org/all/20231011190317.GDZSbxdd5TuCIp5+JN@fat_crate.local/

> 3)
>
> And yes, now that the rush of CPU vulnerabilities seems to be ebbing, we
> should probably consider unifying the existing hodgepodge of mitigation
> Kconfig options as well, to not build up even more technical debt.

What do you mean by unifying the existing hodgepodge of mitigation
Kconfigs? If you are implying to just have fewer config options, I think
that is the opposite of what Linus has suggested previously:

https://lore.kernel.org/all/CAHk-=wjTHeQjsqtHcBGvy9TaJQ5uAm5HrCDuOD9v7qA9U1Xr4w@mail.gmail.com/

> 4)
>
> Fourth, I think we should inform users (in the boot log) when a kernel
> .config changes a mitigation default value compared from what the upstream
> kernel thinks is a suitable default.
>
> Sometimes it can be a simple configuration mistake, or a user might have
> different opinion about the importance of a particular mitigation. Nothing
> heavy-handed, just a simple pr_info() table of changes?

That could be done, but, right now messages are printed in regard to the
mitigations. Aren't these enough?

Here are some examples:

pr_info("MDS: %s\n", mds_strings[mds_mitigation]);
pr_info("TAA: %s\n", taa_strings[taa_mitigation]);
pr_info("MMIO Stale Data: %s\n", mmio_strings[mmio_mitigation]);
pr_info("MMIO Stale Data: Unknown: No mitigations\n");
pr_info("%s\n", srbds_strings[srbds_mitigation]);
pr_info("%s\n", gds_strings[gds_mitigation]);
pr_info("%s\n", spectre_v1_strings[spectre_v1_mitigation]);
pr_info("%s\n", spectre_v2_user_strings[mode]);
pr_info("%s\n", retbleed_strings[retbleed_mitigation]);
pr_info("%s\n", ssb_strings[ssb_mode]);


Thanks for the review!

2023-10-12 16:48:40

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Thu, Oct 12, 2023 at 09:29:20AM +0200, Borislav Petkov wrote:
> On Wed, Oct 11, 2023 at 03:03:25PM -0700, Josh Poimboeuf wrote:
> > I wouldn't call it senseless churn. There are concrete benefits -- for
> > both users and developers -- of having all the mitigation config options
> > living in the same config namespace.
>
> I don't see it. What does the same namespace give you? So you see in the
> code a bunch of ifdeffery and some or all of them have
> CONFIG_MITIGATION_ prepended. To me it doesn't matter whether they're
> mitigations or not - it is just the next Kconfig symbol.

It adds clarity by making the options more self-documenting.

For users who care about such things it makes it easier to identify
which options they need to enable or disable. And it makes it clearer
what those options do without having to go read the docs.

For developers it helps code readability: "ah, this nasty code is for a
side channel mitigation". Also it makes it easier to grep for.

> > Sure, the change might cause pain for users who disable these options,
> > but do those users actually exist?
>
> Apparently there are use cases where people simply want to disable all
> that mitigation crap because they want their performance back. This
> thread being one. And I know of another one with a similar sentiment.

Right, but we have a global option for that. I was wondering if anybody
actually uses the individual options (though I agree with Linus they
should exist to help with code readability).

--
Josh

2023-10-12 17:06:25

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Thu, Oct 12, 2023 at 06:02:41AM -0700, Breno Leitao wrote:
> > Yeah, so this #ifdeffery is unnecessarily ugly - we can actually assign
> > integer values in the Kconfig language and use that for initialization.
> >
> > Is there a reason why we wouldn't want to do something like:
> >
> > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED;
> >
> > ... or so?
>
> Yes. There are two reasons rigth now:
>
> 1) How to avoid the "undefined" behaviour when
> CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED is not defined ? Something as:
>
> error: ‘CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED’ undeclared (first use in this function)
>
> 2) Right now, these _cmd values are all different by default. Here are a few
> examples when the kernel is compiled with the mitigations:
>
> retbleed_cmd = RETBLEED_CMD_AUTO (1)
> spectre_v2_mitigation_cmd = SPECTRE_V2_CMD_AUTO (1)
> ssb_mitigation_cmd = SPEC_STORE_BYPASS_CMD_AUTO (1)
> l1tf_mitigation = L1TF_MITIGATION_FLUSH(2)
> mds_mitigation = MDS_MITIGATION_FULL(1)
> taa_mitigation = TAA_MITIGATION_VERW (2)
> mmio_mitigation = MMIO_MITIGATION_VERW (2)
> gds_mitigation = GDS_MITIGATION_FULL (3)
>
> If there is a solution for 1, then I _think_ we can probably reorder the
> enums, so, the "AUTO" value is always 1?!

I'd rather avoid hard-coding enums as that adds fragility into the mix.

Another way to avoid ifdeffery:

static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;

> > 3)
> >
> > And yes, now that the rush of CPU vulnerabilities seems to be ebbing, we
> > should probably consider unifying the existing hodgepodge of mitigation
> > Kconfig options as well, to not build up even more technical debt.
>
> What do you mean by unifying the existing hodgepodge of mitigation
> Kconfigs? If you are implying to just have fewer config options, I think
> that is the opposite of what Linus has suggested previously:
>
> https://lore.kernel.org/all/CAHk-=wjTHeQjsqtHcBGvy9TaJQ5uAm5HrCDuOD9v7qA9U1Xr4w@mail.gmail.com/

I read that as Ingo agreeing with me that we should rename all the
existing options for consistency.

> > 4)
> >
> > Fourth, I think we should inform users (in the boot log) when a kernel
> > .config changes a mitigation default value compared from what the upstream
> > kernel thinks is a suitable default.
> >
> > Sometimes it can be a simple configuration mistake, or a user might have
> > different opinion about the importance of a particular mitigation. Nothing
> > heavy-handed, just a simple pr_info() table of changes?
>
> That could be done, but, right now messages are printed in regard to the
> mitigations. Aren't these enough?
>
> Here are some examples:
>
> pr_info("MDS: %s\n", mds_strings[mds_mitigation]);
> pr_info("TAA: %s\n", taa_strings[taa_mitigation]);
> pr_info("MMIO Stale Data: %s\n", mmio_strings[mmio_mitigation]);
> pr_info("MMIO Stale Data: Unknown: No mitigations\n");
> pr_info("%s\n", srbds_strings[srbds_mitigation]);
> pr_info("%s\n", gds_strings[gds_mitigation]);
> pr_info("%s\n", spectre_v1_strings[spectre_v1_mitigation]);
> pr_info("%s\n", spectre_v2_user_strings[mode]);
> pr_info("%s\n", retbleed_strings[retbleed_mitigation]);
> pr_info("%s\n", ssb_strings[ssb_mode]);

But notice many/most of those functions exit early if the mitigation is
turned off, thereby skipping the pr_info(). It might be a matter of
just tweaking the print behavior and making it consistent across all the
mitigations.

--
Josh

2023-10-12 18:51:44

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation


* Josh Poimboeuf <[email protected]> wrote:

> On Thu, Oct 12, 2023 at 06:02:41AM -0700, Breno Leitao wrote:
> > > Yeah, so this #ifdeffery is unnecessarily ugly - we can actually assign
> > > integer values in the Kconfig language and use that for initialization.
> > >
> > > Is there a reason why we wouldn't want to do something like:
> > >
> > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED;
> > >
> > > ... or so?
> >
> > Yes. There are two reasons rigth now:
> >
> > 1) How to avoid the "undefined" behaviour when
> > CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED is not defined ? Something as:
> >
> > error: ‘CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED’ undeclared (first use in this function)
> >
> > 2) Right now, these _cmd values are all different by default. Here are a few
> > examples when the kernel is compiled with the mitigations:
> >
> > retbleed_cmd = RETBLEED_CMD_AUTO (1)
> > spectre_v2_mitigation_cmd = SPECTRE_V2_CMD_AUTO (1)
> > ssb_mitigation_cmd = SPEC_STORE_BYPASS_CMD_AUTO (1)
> > l1tf_mitigation = L1TF_MITIGATION_FLUSH(2)
> > mds_mitigation = MDS_MITIGATION_FULL(1)
> > taa_mitigation = TAA_MITIGATION_VERW (2)
> > mmio_mitigation = MMIO_MITIGATION_VERW (2)
> > gds_mitigation = GDS_MITIGATION_FULL (3)
> >
> > If there is a solution for 1, then I _think_ we can probably reorder the
> > enums, so, the "AUTO" value is always 1?!
>
> I'd rather avoid hard-coding enums as that adds fragility into the mix.
>
> Another way to avoid ifdeffery:
>
> static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;

I think we could make it a simple:

static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);

Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
Maybe add a comment to the enum to maintain this property in the future
too.

> > > 3)
> > >
> > > And yes, now that the rush of CPU vulnerabilities seems to be ebbing, we
> > > should probably consider unifying the existing hodgepodge of mitigation
> > > Kconfig options as well, to not build up even more technical debt.
> >
> > What do you mean by unifying the existing hodgepodge of mitigation
> > Kconfigs? If you are implying to just have fewer config options, I think
> > that is the opposite of what Linus has suggested previously:
> >
> > https://lore.kernel.org/all/CAHk-=wjTHeQjsqtHcBGvy9TaJQ5uAm5HrCDuOD9v7qA9U1Xr4w@mail.gmail.com/
>
> I read that as Ingo agreeing with me that we should rename all the
> existing options for consistency.

Yeah, and this is doubly important for security features: inconsistency
invites misunderstandings & bugs ...

> > > Sometimes it can be a simple configuration mistake, or a user might have
> > > different opinion about the importance of a particular mitigation. Nothing
> > > heavy-handed, just a simple pr_info() table of changes?
> >
> > That could be done, but, right now messages are printed in regard to the
> > mitigations. Aren't these enough?
> >
> > Here are some examples:
> >
> > pr_info("MDS: %s\n", mds_strings[mds_mitigation]);
> > pr_info("TAA: %s\n", taa_strings[taa_mitigation]);
> > pr_info("MMIO Stale Data: %s\n", mmio_strings[mmio_mitigation]);
> > pr_info("MMIO Stale Data: Unknown: No mitigations\n");
> > pr_info("%s\n", srbds_strings[srbds_mitigation]);
> > pr_info("%s\n", gds_strings[gds_mitigation]);
> > pr_info("%s\n", spectre_v1_strings[spectre_v1_mitigation]);
> > pr_info("%s\n", spectre_v2_user_strings[mode]);
> > pr_info("%s\n", retbleed_strings[retbleed_mitigation]);
> > pr_info("%s\n", ssb_strings[ssb_mode]);
>
> But notice many/most of those functions exit early if the mitigation is
> turned off, thereby skipping the pr_info(). It might be a matter of
> just tweaking the print behavior and making it consistent across all the
> mitigations.

Yeah. A single consistent table would be the most user-friendly outcome,
with no silence. 'No output' is what kernels without the fixes do too, so
we should always output the chosen bootup status of relevant mitigations.

This would make it a bit easier for users to report bugs too: for example
if some uncommon CPU model number is not enumerated by the mitigations code
and the kernel mistakenly believes that the CPU is safe. While much of this
information can be recovered from the sysfs files too, they are subject to
runtime changes and the values are also not reliably logged at bootup.

Thanks,

Ingo

2023-10-12 20:44:14

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Thu, Oct 12, 2023 at 08:51:24PM +0200, Ingo Molnar wrote:
> > Another way to avoid ifdeffery:
> >
> > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
>
> I think we could make it a simple:
>
> static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);
>
> Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
> Maybe add a comment to the enum to maintain this property in the future
> too.

Hm, that both obfuscates the default and makes it fragile. The fact
that it would need a comment to try to prevent breaking it in the future
is a clue that maybe we shouldn't do it ;-)

--
Josh

2023-10-13 10:51:20

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation


* Josh Poimboeuf <[email protected]> wrote:

> On Thu, Oct 12, 2023 at 08:51:24PM +0200, Ingo Molnar wrote:
> > > Another way to avoid ifdeffery:
> > >
> > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > > IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
> >
> > I think we could make it a simple:
> >
> > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);
> >
> > Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
> > Maybe add a comment to the enum to maintain this property in the future
> > too.
>
> Hm, that both obfuscates the default and makes it fragile. The fact
> that it would need a comment to try to prevent breaking it in the future
> is a clue that maybe we shouldn't do it ;-)

Can be enforced with BUILD_BUG_ON().

Thanks,

Ingo

2023-10-15 14:12:17

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Fri, Oct 13, 2023 at 12:50:59PM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > On Thu, Oct 12, 2023 at 08:51:24PM +0200, Ingo Molnar wrote:
> > > > Another way to avoid ifdeffery:
> > > >
> > > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > > > IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
> > >
> > > I think we could make it a simple:
> > >
> > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);
> > >
> > > Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
> > > Maybe add a comment to the enum to maintain this property in the future
> > > too.
> >
> > Hm, that both obfuscates the default and makes it fragile. The fact
> > that it would need a comment to try to prevent breaking it in the future
> > is a clue that maybe we shouldn't do it ;-)
>
> Can be enforced with BUILD_BUG_ON().

That replaces fragility with brittleness. If we change a default then
we have to go rearrange the corresponding enum, and update the
BUILD_BUG_ONs.

More importantly, it's still less readable because the reader now has to
go read the enum values to cross-reference the hard-coded values of 0
and 1 with the enums which are used everywhere else.

--
Josh

2023-10-15 15:14:13

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Thu, Oct 12, 2023 at 09:48:14AM -0700, Josh Poimboeuf wrote:
> It adds clarity by making the options more self-documenting.

Why would you want to highlight mitigation-specific Kconfig options? You
grep for the symbol and you find what it is.

Why do the mitigation options need to self-document at all?

> For users who care about such things it makes it easier to identify
> which options they need to enable or disable. And it makes it clearer
> what those options do without having to go read the docs.

Sorry, if I see CONFIG_MITIGATE_X and CONFIG_X, I still wanna go see
what X is. And we enable the defaults for users - they don't really
need to read the docs.

> For developers it helps code readability: "ah, this nasty code is for a
> side channel mitigation". Also it makes it easier to grep for.

It doesn't matter - Kconfig options are Kconfig options. We grep
regardless.

> Right, but we have a global option for that. I was wondering if anybody
> actually uses the individual options (though I agree with Linus they
> should exist to help with code readability).

Read Linus' mail - there is some merit to having separate options.

--
Regards/Gruss,
Boris.

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

2023-10-16 09:46:00

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation


* Josh Poimboeuf <[email protected]> wrote:

> On Fri, Oct 13, 2023 at 12:50:59PM +0200, Ingo Molnar wrote:
> >
> > * Josh Poimboeuf <[email protected]> wrote:
> >
> > > On Thu, Oct 12, 2023 at 08:51:24PM +0200, Ingo Molnar wrote:
> > > > > Another way to avoid ifdeffery:
> > > > >
> > > > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > > > > IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
> > > >
> > > > I think we could make it a simple:
> > > >
> > > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);
> > > >
> > > > Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
> > > > Maybe add a comment to the enum to maintain this property in the future
> > > > too.
> > >
> > > Hm, that both obfuscates the default and makes it fragile. The fact
> > > that it would need a comment to try to prevent breaking it in the future
> > > is a clue that maybe we shouldn't do it ;-)
> >
> > Can be enforced with BUILD_BUG_ON().
>
> That replaces fragility with brittleness. If we change a default then
> we have to go rearrange the corresponding enum, and update the
> BUILD_BUG_ONs.

How realistic is that? A world in which an enum named '*_OFF' isn't zero
and the most obvious second enum isn't 'auto' would be unconditionally sad
IMO...

> More importantly, it's still less readable because the reader now has to
> go read the enum values to cross-reference the hard-coded values of 0 and
> 1 with the enums which are used everywhere else.

They'd have to do that anyway, to make sense of the enum jungle.

Thanks,

Ingo

2023-10-16 10:08:12

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation


* Borislav Petkov <[email protected]> wrote:

> On Wed, Oct 11, 2023 at 03:03:25PM -0700, Josh Poimboeuf wrote:
> > I wouldn't call it senseless churn. There are concrete benefits -- for
> > both users and developers -- of having all the mitigation config options
> > living in the same config namespace.
>
> I don't see it. What does the same namespace give you?

Having the current inconsistent Kconfig mess of CPU bug mitigation options
is called 'technical debt', and it is a liability.

Just look at it in its messiness:

CONFIG_RETPOLINE=y
CONFIG_PAGE_TABLE_ISOLATION=y
CONFIG_CPU_UNRET_ENTRY=y
CONFIG_CALL_DEPTH_TRACKING=y
CONFIG_CPU_IBPB_ENTRY=y
CONFIG_CPU_IBRS_ENTRY=y
CONFIG_CPU_SRSO=y
CONFIG_SLS=y

Or current naming is confusing:

- 'CONFIG_PAGE_TABLE_ISOLATION', which works around a CPU bug at hideous
runtime costs switching pagetables at every system call entry and exit,
reads like an isolation feature like CONFIG_CPU_ISOLATION=y, which is a
useful feature that improves worst-case latencies at essentially no
cost ...

- 'CONFIG_CPU_IBPB_ENTRY=y' reads like a useful system call CPU feature.
Nothing in the name tells us that it's a workaround for a CPU bug, with
a significant runtime cost.

- What is CONFIG_SLS? It's a compiler-driven mitigation feature, with a
cost, not a driver for the Space Launch System.

- I could go on. Literally *every* mitigation feature Kconfig name we have
currently is problematic in one fashion or another: and all of them are
whitewashing the fact that they are expensive features to work around
CPU bugs...

All this mess was created due to arguably rushed fix-the-next-vulnerability
time pressures of the last ~2 years, not due to some holy grand design
decisions we wish to preserve as maintainers...

The first 2-3 mitigation options not being harmonized was understandable,
because we really didn't know where it would stop. But we are up to ~8
already.

Unless someone like Linus shuts down this effort with a NAK, this kind of
harmonization would be welcome IMHO:

CONFIG_MITIGATION_PAGE_TABLE_ISOLATION
CONFIG_MITIGATION_RETPOLINE
CONFIG_MITIGATION_UNRET_ENTRY
CONFIG_MITIGATION_CALL_DEPTH_TRACKING
CONFIG_MITIGATION_IBPB_ENTRY
CONFIG_MITIGATION_IBRS_ENTRY
CONFIG_MITIGATION_SRSO
CONFIG_MITIGATION_SLS

Efforts by competent people fixing this technical debt comprehensively are
welcome in my book, as long as advantages are substantial and the
inter-tree churn is not prohibitive - which I believe is true in this case,
and churn effect can be reduced via smart timing of rename-patches in any case.

Thanks,

Ingo

2023-10-16 12:47:27

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Mon, Oct 16, 2023 at 12:07:59PM +0200, Ingo Molnar wrote:
> Unless someone like Linus shuts down this effort with a NAK, this kind of
> harmonization would be welcome IMHO:

Harmonization being done for whom?

Because I doubt users read Kconfig names. So you must be wanting it for
us, who do stare at that code.

Which brings me to my initial question from uptread:

"What does the same namespace give you? So you see in the code a bunch
of ifdeffery and some or all of them have CONFIG_MITIGATION_ prepended.
To me it doesn't matter whether they're mitigations or not - it is just
the next Kconfig symbol."

IOW, I don't see the particular reason for having those namespaced. But
since you and Josh care so much and I don't - not that much :-) - and
I definitely won't be doing the work, feel free.

Thx.

--
Regards/Gruss,
Boris.

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

2023-10-17 15:08:59

by Breno Leitao

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Mon, Oct 16, 2023 at 02:46:54PM +0200, Borislav Petkov wrote:
> IOW, I don't see the particular reason for having those namespaced. But
> since you and Josh care so much and I don't - not that much :-) - and
> I definitely won't be doing the work, feel free.

Thanks for all the discussion so far.

I am summarizing what I got from this discussion so far, and
starting to get the direction on where we want to be:

0) Each mitigation should have their own Kconfig entry under
SPECULATION_MITIGATIONS.

1) Have "Mitigation" namespace for each mitigation. These are the new
name for the mitigations:
* MITIGATION_PAGE_TABLE_ISOLATION
* MITIGATION_RETPOLINE
* MITIGATION_UNRET_ENTRY
* MITIGATION_CALL_DEPTH_TRACKING
* MITIGATION_IBPB_ENTRY
* MITIGATION_IBRS_ENTRY
* MITIGATION_SRSO
* MITIGATION_SLS

2) Split the GDS mitigation in two:
* MITIGATION_GDS
* MITIGATION_GDS_FORCE

3) Create new Kconfigs for the following missing mitigations:
* MITIGATION_MDS
* MITIGATION_TAA
* MITIGATION_MMIO_STALE_DATA
* MITIGATION_L1TF
* MITIGATION_RETBLEED
* MITIGATION_SPECTRE_V1
* MITIGATION_SPECTRE_V2
* MITIGATION_SRBDS
* MITIGATION_SSB

4) Do no touch the enum order, instead use the something as the code
below to avoid the #ifdefs in the code:

static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;

5) Make sure that there is clear messages when the mitigations are
disabled. i.e, make sure that the current pr_info() we have today are
called in case the mitigation is disabled.

Am I missing something?

2023-10-17 17:15:46

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation

On Mon, Oct 16, 2023 at 11:45:38AM +0200, Ingo Molnar wrote:
> * Josh Poimboeuf <[email protected]> wrote:
> > On Fri, Oct 13, 2023 at 12:50:59PM +0200, Ingo Molnar wrote:
> > > * Josh Poimboeuf <[email protected]> wrote:
> > > > On Thu, Oct 12, 2023 at 08:51:24PM +0200, Ingo Molnar wrote:
> > > > > > Another way to avoid ifdeffery:
> > > > > >
> > > > > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > > > > > IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
> > > > >
> > > > > I think we could make it a simple:
> > > > >
> > > > > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = IS_ENABLED(CONFIG_MITIGATION_RETBLEED);
> > > > >
> > > > > Because RETBLEED_CMD_AUTO && RETBLEED_CMD_OFF maps naturally to 1 and 0.
> > > > > Maybe add a comment to the enum to maintain this property in the future
> > > > > too.
> > > >
> > > > Hm, that both obfuscates the default and makes it fragile. The fact
> > > > that it would need a comment to try to prevent breaking it in the future
> > > > is a clue that maybe we shouldn't do it ;-)
> > >
> > > Can be enforced with BUILD_BUG_ON().
> >
> > That replaces fragility with brittleness. If we change a default then
> > we have to go rearrange the corresponding enum, and update the
> > BUILD_BUG_ONs.
>
> How realistic is that? A world in which an enum named '*_OFF' isn't zero
> and the most obvious second enum isn't 'auto' would be unconditionally sad
> IMO...

I'm glad you asked ;-)

Not only is it realistic, it already seems to be the case for the
majority of the enums which are used for defaults:

enum l1tf_mitigations {
L1TF_MITIGATION_OFF,
L1TF_MITIGATION_FLUSH_NOWARN,
L1TF_MITIGATION_FLUSH, /* default */
L1TF_MITIGATION_FLUSH_NOSMT,
L1TF_MITIGATION_FULL,
L1TF_MITIGATION_FULL_FORCE
};

enum taa_mitigations {
TAA_MITIGATION_OFF,
TAA_MITIGATION_UCODE_NEEDED,
TAA_MITIGATION_VERW, /* default */
TAA_MITIGATION_TSX_DISABLED,
};

enum mmio_mitigations {
MMIO_MITIGATION_OFF,
MMIO_MITIGATION_UCODE_NEEDED,
MMIO_MITIGATION_VERW, /* default */
};

enum srbds_mitigations {
SRBDS_MITIGATION_OFF,
SRBDS_MITIGATION_UCODE_NEEDED,
SRBDS_MITIGATION_FULL, /* default */
SRBDS_MITIGATION_TSX_OFF,
SRBDS_MITIGATION_HYPERVISOR,
};

enum l1d_flush_mitigations {
L1D_FLUSH_OFF = 0, / * default */
L1D_FLUSH_ON,
};

enum gds_mitigations {
GDS_MITIGATION_OFF,
GDS_MITIGATION_UCODE_NEEDED,
GDS_MITIGATION_FORCE,
GDS_MITIGATION_FULL, /* default */
GDS_MITIGATION_FULL_LOCKED,
GDS_MITIGATION_HYPERVISOR,
};

enum srso_mitigation_cmd {
SRSO_CMD_OFF,
SRSO_CMD_MICROCODE,
SRSO_CMD_SAFE_RET, /* default */
SRSO_CMD_IBPB,
SRSO_CMD_IBPB_ON_VMEXIT,
};

It's dangerous (and in fact antithetical to the concept of enums!) to
make assumptions about enum values, for both existing and future code.

> > More importantly, it's still less readable because the reader now has to
> > go read the enum values to cross-reference the hard-coded values of 0 and
> > 1 with the enums which are used everywhere else.
>
> They'd have to do that anyway, to make sense of the enum jungle.

Why? The enum values are completely opaque to the code as far as I can
tell. Changing that will just make the code even more obtuse.

I don't see any benefit in hard-coding them. What's the point?

--
Josh