linux-kernel - Re: [PATCH v3 10/35] x86/bugs: Restructure gds mitigation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+i-1C2LD1A-QjTmGv-ONB-1+jmecZSU7i__G7gf=81VUNMKCg@mail.gmail.com>
Date: Mon, 10 Feb 2025 18:06:20 +0100
From: Brendan Jackman <jackmanb@...gle.com>
To: David Kaplan <david.kaplan@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, 
	Peter Zijlstra <peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>, 
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 10/35] x86/bugs: Restructure gds mitigation

On Wed, 8 Jan 2025 at 21:28, David Kaplan <david.kaplan@....com> wrote:
>
> Restructure gds mitigation to use select/apply functions to create
> consistent vulnerability handling.
>
> Define new AUTO mitigation for gds.
>
> Signed-off-by: David Kaplan <david.kaplan@....com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index fedd693b2218..58ac99b74bd3 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -76,6 +76,7 @@ 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 gds_select_mitigation(void);
> +static void __init gds_apply_mitigation(void);
>
>  /* The base value of the SPEC_CTRL MSR without task-specific bits set */
>  u64 x86_spec_ctrl_base;
> @@ -220,6 +221,7 @@ void __init cpu_select_mitigations(void)
>         mmio_apply_mitigation();
>         rfds_apply_mitigation();
>         srbds_apply_mitigation();
> +       gds_apply_mitigation();
>  }
>
>  /*
> @@ -811,6 +813,7 @@ early_param("l1d_flush", l1d_flush_parse_cmdline);
>
>  enum gds_mitigations {
>         GDS_MITIGATION_OFF,
> +       GDS_MITIGATION_AUTO,
>         GDS_MITIGATION_UCODE_NEEDED,
>         GDS_MITIGATION_FORCE,
>         GDS_MITIGATION_FULL,
> @@ -819,7 +822,7 @@ enum gds_mitigations {
>  };
>
>  static enum gds_mitigations gds_mitigation __ro_after_init =
> -       IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_FULL : GDS_MITIGATION_OFF;
> +       IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_AUTO : GDS_MITIGATION_OFF;
>
>  static const char * const gds_strings[] = {
>         [GDS_MITIGATION_OFF]            = "Vulnerable",
> @@ -860,6 +863,7 @@ void update_gds_msr(void)
>         case GDS_MITIGATION_FORCE:
>         case GDS_MITIGATION_UCODE_NEEDED:
>         case GDS_MITIGATION_HYPERVISOR:
> +       case GDS_MITIGATION_AUTO:
>                 return;
>         }
>
> @@ -883,13 +887,16 @@ static void __init gds_select_mitigation(void)
>
>         if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
>                 gds_mitigation = GDS_MITIGATION_HYPERVISOR;
> -               goto out;
> +               return;
>         }
>
>         if (cpu_mitigations_off())
>                 gds_mitigation = GDS_MITIGATION_OFF;
>         /* Will verify below that mitigation _can_ be disabled */
>
> +       if (gds_mitigation == GDS_MITIGATION_AUTO)
> +               gds_mitigation = GDS_MITIGATION_FULL;
> +
>         /* No microcode */
>         if (!(x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)) {
>                 if (gds_mitigation == GDS_MITIGATION_FORCE) {
> @@ -902,7 +909,7 @@ static void __init gds_select_mitigation(void)
>                 } else {
>                         gds_mitigation = GDS_MITIGATION_UCODE_NEEDED;
>                 }
> -               goto out;
> +               return;
>         }
>
>         /* Microcode has mitigation, use it */
> @@ -923,9 +930,16 @@ static void __init gds_select_mitigation(void)
>                  */
>                 gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
>         }
> +}
> +
> +static void __init gds_apply_mitigation(void)
> +{
> +       if (!boot_cpu_has_bug(X86_BUG_GDS))
> +               return;
> +
> +       if (x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)
> +               update_gds_msr();

IMO it's a shame to be looking at MSR bits in here instead of just
relying on the direct output of the select/update functions.

I think in this case we can just remove the conditional since if
!ARCH_CAP_GDS_CTRL then gds_mitigation must be FORCE or UCODE_NEEDED
in which case update_gds_msr() is a nop.

Now I make these comments I realise maybe my expectation about these
three functions is not actually the same as yours. Here's how I
envisaged your design:

- select: Look around at the hardware and the cmdline and decide what
we think we wanna do in fairly abstract terms. Record that result in
*_mitigation.

- update: Look around at the other mitigations and potentially change
our mind (or perhaps just update *_mitigation to reflect mitigation
that is being done regardless for other vulns, which also mitigate
this vuln).

- apply: Poke the hardware/cap flags/static keys/etc to actuate the
decision we made in the previous steps.

Let me know if that's aligned with your vision or not.