Re: [PATCH v5 00/16] Attack vector controls (part 1)

Ingo Molnar
2025-04-18T20:03:46Z

* 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