arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/cpu/bugs.c | 1112 +++++++++++++++++------------- arch/x86/kvm/vmx/vmx.c | 2 + 3 files changed, 644 insertions(+), 471 deletions(-)
This is an updated version of the first half of the attack vector series which focuses on restructuring arch/x86/kernel/cpu/bugs.c. For more info the attack vector series, please see v4 at https://lore.kernel.org/all/20250310164023.779191-1-david.kaplan@amd.com/. These patches restructure the existing mitigation selection logic to use a uniform set of functions. First, the "select" function is called for each mitigation to select an appropriate mitigation. Unless a mitigation is explicitly selected or disabled with a command line option, the default mitigation is AUTO and the "select" function will then choose the best mitigation. After the "select" function is called for each mitigation, some mitigations define an "update" function which can be used to update the selection, based on the choices made by other mitigations. Finally, the "apply" function is called which enables the chosen mitigation. This structure simplifies the mitigation control logic, especially when there are dependencies between multiple vulnerabilities. This is mostly code restructuring without functional changes, except where noted. Compared to v4 this only includes bug fixes/cleanup. David Kaplan (16): x86/bugs: Restructure MDS mitigation x86/bugs: Restructure TAA mitigation x86/bugs: Restructure MMIO mitigation x86/bugs: Restructure RFDS mitigation x86/bugs: Remove md_clear_*_mitigation() x86/bugs: Restructure SRBDS mitigation x86/bugs: Restructure GDS mitigation x86/bugs: Restructure spectre_v1 mitigation x86/bugs: Allow retbleed=stuff only on Intel x86/bugs: Restructure retbleed mitigation x86/bugs: Restructure spectre_v2_user mitigation x86/bugs: Restructure BHI mitigation x86/bugs: Restructure spectre_v2 mitigation x86/bugs: Restructure SSB mitigation x86/bugs: Restructure L1TF mitigation x86/bugs: Restructure SRSO mitigation arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/cpu/bugs.c | 1112 +++++++++++++++++------------- arch/x86/kvm/vmx/vmx.c | 2 + 3 files changed, 644 insertions(+), 471 deletions(-) base-commit: 33aa28024418782f644d8924026f1db21b3354a6 -- 2.34.1
On Fri, Apr 18, 2025 at 11:17:05AM -0500, David Kaplan wrote: > This is an updated version of the first half of the attack vector series > which focuses on restructuring arch/x86/kernel/cpu/bugs.c. > > For more info the attack vector series, please see v4 at > https://lore.kernel.org/all/20250310164023.779191-1-david.kaplan@amd.com/. > > These patches restructure the existing mitigation selection logic to use a > uniform set of functions. First, the "select" function is called for each > mitigation to select an appropriate mitigation. Unless a mitigation is > explicitly selected or disabled with a command line option, the default > mitigation is AUTO and the "select" function will then choose the best > mitigation. After the "select" function is called for each mitigation, > some mitigations define an "update" function which can be used to update > the selection, based on the choices made by other mitigations. Finally, > the "apply" function is called which enables the chosen mitigation. > > This structure simplifies the mitigation control logic, especially when > there are dependencies between multiple vulnerabilities. > > This is mostly code restructuring without functional changes, except where > noted. > > Compared to v4 this only includes bug fixes/cleanup. Reviewed-by: Josh Poimboeuf <jpoimboe@kernel.org> -- Josh
* David Kaplan <david.kaplan@amd.com> wrote:
> This is an updated version of the first half of the attack vector series
> which focuses on restructuring arch/x86/kernel/cpu/bugs.c.
>
> For more info the attack vector series, please see v4 at
> https://lore.kernel.org/all/20250310164023.779191-1-david.kaplan@amd.com/.
>
> These patches restructure the existing mitigation selection logic to use a
> uniform set of functions. First, the "select" function is called for each
> mitigation to select an appropriate mitigation. Unless a mitigation is
> explicitly selected or disabled with a command line option, the default
> mitigation is AUTO and the "select" function will then choose the best
> mitigation. After the "select" function is called for each mitigation,
> some mitigations define an "update" function which can be used to update
> the selection, based on the choices made by other mitigations. Finally,
> the "apply" function is called which enables the chosen mitigation.
>
> This structure simplifies the mitigation control logic, especially when
> there are dependencies between multiple vulnerabilities.
>
> This is mostly code restructuring without functional changes, except where
> noted.
>
> Compared to v4 this only includes bug fixes/cleanup.
>
> David Kaplan (16):
> x86/bugs: Restructure MDS mitigation
> x86/bugs: Restructure TAA mitigation
> x86/bugs: Restructure MMIO mitigation
> x86/bugs: Restructure RFDS mitigation
> x86/bugs: Remove md_clear_*_mitigation()
> x86/bugs: Restructure SRBDS mitigation
> x86/bugs: Restructure GDS mitigation
> x86/bugs: Restructure spectre_v1 mitigation
> x86/bugs: Allow retbleed=stuff only on Intel
> x86/bugs: Restructure retbleed mitigation
> x86/bugs: Restructure spectre_v2_user mitigation
> x86/bugs: Restructure BHI mitigation
> x86/bugs: Restructure spectre_v2 mitigation
> x86/bugs: Restructure SSB mitigation
> x86/bugs: Restructure L1TF mitigation
> x86/bugs: Restructure SRSO mitigation
>
> arch/x86/include/asm/processor.h | 1 +
> arch/x86/kernel/cpu/bugs.c | 1112 +++++++++++++++++-------------
> arch/x86/kvm/vmx/vmx.c | 2 +
> 3 files changed, 644 insertions(+), 471 deletions(-)
So I really like this cleanup & restructuring.
A namespace suggestion.
Instead of _op_mitigation postfixes:
static void __init spectre_v1_select_mitigation(void);
static void __init spectre_v1_apply_mitigation(void);
static void __init spectre_v2_select_mitigation(void);
static void __init retbleed_select_mitigation(void);
static void __init retbleed_update_mitigation(void);
static void __init retbleed_apply_mitigation(void);
static void __init spectre_v2_user_select_mitigation(void);
static void __init spectre_v2_user_update_mitigation(void);
static void __init spectre_v2_user_apply_mitigation(void);
static void __init ssb_select_mitigation(void);
static void __init ssb_apply_mitigation(void);
static void __init l1tf_select_mitigation(void);
static void __init l1tf_apply_mitigation(void);
static void __init mds_select_mitigation(void);
static void __init mds_update_mitigation(void);
static void __init mds_apply_mitigation(void);
static void __init taa_select_mitigation(void);
static void __init taa_update_mitigation(void);
static void __init taa_apply_mitigation(void);
static void __init mmio_select_mitigation(void);
static void __init mmio_update_mitigation(void);
static void __init mmio_apply_mitigation(void);
static void __init rfds_select_mitigation(void);
static void __init rfds_update_mitigation(void);
static void __init rfds_apply_mitigation(void);
static void __init srbds_select_mitigation(void);
static void __init srbds_apply_mitigation(void);
static void __init l1d_flush_select_mitigation(void);
static void __init srso_select_mitigation(void);
static void __init srso_update_mitigation(void);
static void __init srso_apply_mitigation(void);
static void __init gds_select_mitigation(void);
static void __init gds_apply_mitigation(void);
static void __init bhi_select_mitigation(void);
static void __init bhi_update_mitigation(void);
static void __init bhi_apply_mitigation(void);
Wouldn't it be nicer to have mitigation_op_ prefixes, like most kernel
subsystems use for their function names:
static void __init mitigation_select_spectre_v1(void);
static void __init mitigation_enable_spectre_v1(void);
static void __init mitigation_select_spectre_v2(void);
static void __init mitigation_select_retbleed(void);
static void __init mitigation_update_retbleed(void);
static void __init mitigation_enable_retbleed(void);
static void __init mitigation_select_spectre_v2_user(void);
static void __init mitigation_update_spectre_v2_user(void);
static void __init mitigation_enable_spectre_v2_user(void);
static void __init mitigation_select_ssb(void);
static void __init mitigation_enable_ssb(void);
static void __init mitigation_select_l1tf(void);
static void __init mitigation_enable_l1tf(void);
static void __init mitigation_select_mds(void);
static void __init mitigation_update_mds(void);
static void __init mitigation_enable_mds(void);
static void __init mitigation_select_taa(void);
static void __init mitigation_update_taa(void);
static void __init mitigation_enable_taa(void);
static void __init mitigation_select_mmio(void);
static void __init mitigation_update_mmio(void);
static void __init mitigation_enable_mmio(void);
static void __init mitigation_select_rfds(void);
static void __init mitigation_update_rfds(void);
static void __init mitigation_enable_rfds(void);
static void __init mitigation_select_srbds(void);
static void __init mitigation_enable_srbds(void);
static void __init mitigation_select_l1d_flush(void);
static void __init mitigation_select_srso(void);
static void __init mitigation_update_srso(void);
static void __init mitigation_enable_srso(void);
static void __init mitigation_select_gds(void);
static void __init mitigation_enable_gds(void);
static void __init mitigation_select_bhi(void);
static void __init mitigation_update_bhi(void);
static void __init mitigation_enable_bhi(void);
(Note that I changed '_apply' to '_enable', to get three 6-letter verbs. ;-)
We already do this for the Kconfig knobs:
CONFIG_MITIGATION_PAGE_TABLE_ISOLATION=y
CONFIG_MITIGATION_RETPOLINE=y
CONFIG_MITIGATION_RETHUNK=y
CONFIG_MITIGATION_UNRET_ENTRY=y
CONFIG_MITIGATION_CALL_DEPTH_TRACKING=y
CONFIG_MITIGATION_IBPB_ENTRY=y
CONFIG_MITIGATION_IBRS_ENTRY=y
CONFIG_MITIGATION_SRSO=y
CONFIG_MITIGATION_SLS=y
CONFIG_MITIGATION_GDS=y
CONFIG_MITIGATION_RFDS=y
CONFIG_MITIGATION_SPECTRE_BHI=y
CONFIG_MITIGATION_MDS=y
CONFIG_MITIGATION_TAA=y
CONFIG_MITIGATION_MMIO_STALE_DATA=y
CONFIG_MITIGATION_L1TF=y
CONFIG_MITIGATION_RETBLEED=y
CONFIG_MITIGATION_SPECTRE_V1=y
CONFIG_MITIGATION_SPECTRE_V2=y
CONFIG_MITIGATION_SRBDS=y
CONFIG_MITIGATION_SSB=y
and in particular when these functions are used in blocks (as they
often are), it looks much cleaner and more organized:
# Before:
/* Select the proper CPU mitigations before patching alternatives: */
spectre_v1_select_mitigation();
spectre_v2_select_mitigation();
retbleed_select_mitigation();
spectre_v2_user_select_mitigation();
ssb_select_mitigation();
l1tf_select_mitigation();
mds_select_mitigation();
taa_update_mitigation();
taa_select_mitigation();
mmio_select_mitigation();
rfds_select_mitigation();
srbds_select_mitigation();
l1d_flush_select_mitigation();
srso_select_mitigation();
gds_select_mitigation();
bhi_select_mitigation();
# After:
/* Select the proper CPU mitigations before patching alternatives: */
mitigation_select_spectre_v1();
mitigation_select_spectre_v2();
mitigation_select_retbleed();
mitigation_select_spectre_v2_user();
mitigation_select_ssb();
mitigation_select_l1tf();
mitigation_select_mds();
mitigation_update_taa();
mitigation_select_taa();
mitigation_select_mmio();
mitigation_select_rfds();
mitigation_select_srbds();
mitigation_select_l1d_flush();
mitigation_select_srso();
mitigation_select_gds();
mitigation_select_bhi();
Right?
Bonus quiz: I've snuck a subtle bug into the code sequence above. In
which block is it easier to find visually? :-)
Thanks,
Ingo
On Fri, Apr 18, 2025 at 10:03:42PM +0200, Ingo Molnar wrote:
> /* Select the proper CPU mitigations before patching alternatives: */
> mitigation_select_spectre_v1();
> mitigation_select_spectre_v2();
> mitigation_select_retbleed();
> mitigation_select_spectre_v2_user();
> mitigation_select_ssb();
> mitigation_select_l1tf();
> mitigation_select_mds();
> mitigation_update_taa();
> mitigation_select_taa();
> mitigation_select_mmio();
> mitigation_select_rfds();
> mitigation_select_srbds();
> mitigation_select_l1d_flush();
> mitigation_select_srso();
> mitigation_select_gds();
> mitigation_select_bhi();
The bad side of that is that you have a whole set of letters
- "mitigation_select" - before the *actual* name which is the only thing one
is interested in. With the vectors, one is now interested in the operation too
- select, update or apply.
I'd make *which* mitigation is a lot more visible instead of that useless
amassing of letters - especially since all those functions are internal.
spectre_v1_select()
spectre_v2_select()
...
then
...
spectre_v1_update()
...
mmio_update()
and then
...
srso_apply()
l1tf_apply()
...
and so on. Short, sweet, not a lot of bla, that useless "mitigation"
regurgitating is gone and the code is readable again.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
* Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Apr 18, 2025 at 10:03:42PM +0200, Ingo Molnar wrote:
> > /* Select the proper CPU mitigations before patching alternatives: */
> > mitigation_select_spectre_v1();
> > mitigation_select_spectre_v2();
> > mitigation_select_retbleed();
> > mitigation_select_spectre_v2_user();
> > mitigation_select_ssb();
> > mitigation_select_l1tf();
> > mitigation_select_mds();
> > mitigation_update_taa();
> > mitigation_select_taa();
> > mitigation_select_mmio();
> > mitigation_select_rfds();
> > mitigation_select_srbds();
> > mitigation_select_l1d_flush();
> > mitigation_select_srso();
> > mitigation_select_gds();
> > mitigation_select_bhi();
>
> The bad side of that is that you have a whole set of letters
> - "mitigation_select" - before the *actual* name which is the only thing one
> is interested in. With the vectors, one is now interested in the operation too
> - select, update or apply.
I have three counter-arguments:
1)
The above pattern is not a big problem really, as the human brain has
no trouble ignoring well-structured syntactic repetitions on the left
side and will focus on the right side column.
Unlike the current status quo, which your reply didn't quote, so I'll
quote it again:
static void __init spectre_v1_select_mitigation(void);
static void __init spectre_v2_select_mitigation(void);
static void __init retbleed_select_mitigation(void);
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);
static void __init srbds_select_mitigation(void);
static void __init l1d_flush_select_mitigation(void);
static void __init srso_select_mitigation(void);
static void __init gds_select_mitigation(void);
Which is far more visually messy.
2)
As to shortening the function names to reduce (but not eliminate ...)
the mess:
That extra mitigation_ prefix or _mitigation postfix might sound like
repetitious when used in mass-calls like above - but it's really useful
when reading the actual definitions:
> Short, sweet,
... there's such a thing as too short, too sweet, too ambiguous, which
is why we ended up with the current name of gds_select_mitigation() et
al to begin with.
But let's see an example. Consider:
static void __init gds_select(void)
{
u64 mcu_ctrl;
What do I select? Some GDS detail? Or the main mitigation itself?
Nothing really tells me.
While with:
static void __init mitigation_select_gds(void)
{
u64 mcu_ctrl;
It's immediately clear that this is the main function that selects the
GDS mitigation.
3)
A proper namespace also makes it *much* easier to grep for specific
primitives.
With your suggested 'gds_select()' naming, if I want to search for all
gds_ primitives, I get:
starship:~/tip> git grep gds_ arch/x86/kernel/cpu/bugs.c
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void);
arch/x86/kernel/cpu/bugs.c: gds_select_mitigation();
arch/x86/kernel/cpu/bugs.c:enum gds_mitigations {
arch/x86/kernel/cpu/bugs.c:static enum gds_mitigations gds_mitigation __ro_after_init =
arch/x86/kernel/cpu/bugs.c:static const char * const gds_strings[] = {
arch/x86/kernel/cpu/bugs.c:bool gds_ucode_mitigated(void)
arch/x86/kernel/cpu/bugs.c: return (gds_mitigation == GDS_MITIGATION_FULL ||
arch/x86/kernel/cpu/bugs.c: gds_mitigation == GDS_MITIGATION_FULL_LOCKED);
arch/x86/kernel/cpu/bugs.c:EXPORT_SYMBOL_GPL(gds_ucode_mitigated);
arch/x86/kernel/cpu/bugs.c:void update_gds_msr(void)
arch/x86/kernel/cpu/bugs.c: switch (gds_mitigation) {
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_HYPERVISOR;
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_OFF;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_FORCE) {
arch/x86/kernel/cpu/bugs.c: * here rather than in update_gds_msr()
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_UCODE_NEEDED;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_FORCE)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FULL;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_OFF)
arch/x86/kernel/cpu/bugs.c: * but others are then update_gds_msr() will WARN() of the state
arch/x86/kernel/cpu/bugs.c: * mismatch. If the boot CPU is locked update_gds_msr() will
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
arch/x86/kernel/cpu/bugs.c: update_gds_msr();
arch/x86/kernel/cpu/bugs.c: pr_info("%s\n", gds_strings[gds_mitigation]);
arch/x86/kernel/cpu/bugs.c:static int __init gds_parse_cmdline(char *str)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_OFF;
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FORCE;
arch/x86/kernel/cpu/bugs.c:early_param("gather_data_sampling", gds_parse_cmdline);
arch/x86/kernel/cpu/bugs.c:static ssize_t gds_show_state(char *buf)
arch/x86/kernel/cpu/bugs.c: return sysfs_emit(buf, "%s\n", gds_strings[gds_mitigation]);
arch/x86/kernel/cpu/bugs.c: return gds_show_state(buf);
Or, if I limit this to function calls only:
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void);
arch/x86/kernel/cpu/bugs.c: gds_select_mitigation();
arch/x86/kernel/cpu/bugs.c:bool gds_ucode_mitigated(void)
arch/x86/kernel/cpu/bugs.c:void update_gds_msr(void)
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void)
arch/x86/kernel/cpu/bugs.c: * here rather than in update_gds_msr()
arch/x86/kernel/cpu/bugs.c: * but others are then update_gds_msr() will WARN() of the state
arch/x86/kernel/cpu/bugs.c: * mismatch. If the boot CPU is locked update_gds_msr() will
arch/x86/kernel/cpu/bugs.c: update_gds_msr();
arch/x86/kernel/cpu/bugs.c:static int __init gds_parse_cmdline(char *str)
arch/x86/kernel/cpu/bugs.c:static ssize_t gds_show_state(char *buf)
arch/x86/kernel/cpu/bugs.c: return gds_show_state(buf);
Versus a targeted, obvious, untuitive search for all mitigation_
functions related to GDS:
starship:~/tip> git grep -E 'mitigation_.*_gds' arch/x86/kernel/cpu/bugs.c
arch/x86/kernel/cpu/bugs.c:static void __init mitigation_select_gds(void);
arch/x86/kernel/cpu/bugs.c: mitigation_select_gds();
arch/x86/kernel/cpu/bugs.c:static void __init mitigation_select_gds(void)
Because a proper namespace is a far more easier to search.
Hierarchical lexicographic organization of function names is basically
a code organization 101 concept, and I didn't think it would be
particularly controversial. :-)
Thanks,
Ingo
On Tue, Apr 22, 2025 at 11:46:33AM +0200, Ingo Molnar wrote:
> The above pattern is not a big problem really, as the human brain has
> no trouble ignoring well-structured syntactic repetitions on the left
> side and will focus on the right side column.
>
> Unlike the current status quo, which your reply didn't quote, so I'll
> quote it again:
>
> static void __init spectre_v1_select_mitigation(void);
> static void __init spectre_v2_select_mitigation(void);
> static void __init retbleed_select_mitigation(void);
> 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);
> static void __init srbds_select_mitigation(void);
> static void __init l1d_flush_select_mitigation(void);
> static void __init srso_select_mitigation(void);
> static void __init gds_select_mitigation(void);
>
> Which is far more visually messy.
That's when those are written in a block together - there the human
brain can selectively ignore.
I mean when the functions are called in succession. See
cpu_select_mitigations(). All this function does is select mitigations.
So there's no need to state the selection of every single mitigation.
> What do I select? Some GDS detail? Or the main mitigation itself?
> Nothing really tells me.
We're going to have _select(), _update() and apply() function per
mitigation. And this will be documented at the top of bugs.c
In the confines of this file, those functions are special and when you
select, update or apply something in bugs.c, it should always refer to
a mitigation.
We can make that decision here, in that file, for the sanity of everyone
who's looking at it.
> While with:
>
> static void __init mitigation_select_gds(void)
> {
> u64 mcu_ctrl;
>
> It's immediately clear that this is the main function that selects the
> GDS mitigation.
Sorry:
$ git grep -o -i mitigation arch/x86/kernel/cpu/bugs.c | wc -l
714
That's just madness: this file has waaaaay too many "mitigation"s and it
impairs the reading. *Especially* if this file is *all* *about*
mitigations and only about that.
The new scheme is going to tip those scales over 1K...
> 3)
>
> A proper namespace also makes it *much* easier to grep for specific
> primitives.
>
> With your suggested 'gds_select()' naming, if I want to search for all
> gds_ primitives, I get:
Sorry, this is a bad example: if you want to search for a static
function's uses in the same file, you simply go in your favorite editor
and search for "word-under-cursor".
> Hierarchical lexicographic organization of function names is basically
> a code organization 101 concept, and I didn't think it would be
> particularly controversial. :-)
And yet the current naming ain't optimal, IMO, due to the too often
regurgitation of "mitigation". And that's particularly a problem if
it impairs reading of the code - lexicographic organization then is
secondary.
And as said before, you don't need a "mitigation_" prefix for static
functions.
And reading that code properly is the most important thing. *Especially*
*that* code.
So the goal of the bikeshedding here should be optimal reading. I.e.,
what function and variable naming allows for an optimal reading of the
code.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
© 2016 - 2025 Red Hat, Inc.