Message-ID:
 <LV3PR12MB92653811577802E84DEB6ACE9493A@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Fri, 16 May 2025 15:06:26 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
CC: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>,
	Ingo Molnar <mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] x86/bugs: Restructure ITS mitigation

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> Sent: Thursday, May 15, 2025 6:52 PM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Borislav Petkov <bp@...en8.de>;
> Peter Zijlstra <peterz@...radead.org>; Josh Poimboeuf <jpoimboe@...nel.org>;
> 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] x86/bugs: Restructure ITS mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, May 15, 2025 at 08:47:56AM -0500, David Kaplan wrote:
> > Restructure the ITS mitigation to use select/update/apply functions
> > like the other mitigations.
> >
> > There is a particularly complex interaction between ITS and Retbleed
> > as CDT (Call Depth Tracking) is a mitigation for both, and either
> > its=stuff or retbleed=stuff will attempt to enable CDT.
> >
> > retbleed_update_mitigation() runs first and will check the necessary
> > pre-conditions for CDT if either ITS or Retbleed stuffing is selected.
> > If checks pass and ITS stuffing is selected, it will select stuffing
> > for Retbleed as well.
> >
> > its_update_mitigation() runs after and will either select stuffing if
> > retbleed stuffing was enabled, or fall back to the default (aligned
> > thunks) if stuffing could not be enabled.
> >
> > Enablement of CDT is done exclusively in retbleed_apply_mitigation().
> > its_apply_mitigation() is only used to enable aligned thunks.
> >
> > Signed-off-by: David Kaplan <david.kaplan@....com>
> >
> > @@ -1125,6 +1133,14 @@ enum retbleed_mitigation {
> >       RETBLEED_MITIGATION_STUFF,
> >  };
> >
> > +enum its_mitigation {
> > +     ITS_MITIGATION_OFF,
> > +     ITS_MITIGATION_AUTO,
> > +     ITS_MITIGATION_VMEXIT_ONLY,
> > +     ITS_MITIGATION_ALIGNED_THUNKS,
> > +     ITS_MITIGATION_RETPOLINE_STUFF,
> > +};
>
> This is in between retbleed declarations, I would suggest to move this before
> retbleed mitigation starts.

Ok

>
> enum its_mitigation {
>         ITS_MITIGATION_OFF,
>         ITS_MITIGATION_AUTO,
>         ITS_MITIGATION_VMEXIT_ONLY,
>         ITS_MITIGATION_ALIGNED_THUNKS,
>         ITS_MITIGATION_RETPOLINE_STUFF,
> };
>
> static enum its_mitigation its_mitigation __ro_after_init =
>         IS_ENABLED(CONFIG_MITIGATION_ITS) ? ITS_MITIGATION_AUTO :
> ITS_MITIGATION_OFF;
>
> #undef pr_fmt
> #define pr_fmt(fmt)     "RETBleed: " fmt
>
> enum retbleed_mitigation {
>
> >  static const char * const retbleed_strings[] = {
> >       [RETBLEED_MITIGATION_NONE]      = "Vulnerable",
> >       [RETBLEED_MITIGATION_UNRET]     = "Mitigation: untrained return thunk",
> > @@ -1137,6 +1153,9 @@ static const char * const retbleed_strings[] = {
> > static enum retbleed_mitigation retbleed_mitigation __ro_after_init =
> >       IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ?
> > RETBLEED_MITIGATION_AUTO : RETBLEED_MITIGATION_NONE;
> >
> > +static enum its_mitigation its_mitigation __ro_after_init =
> > +     IS_ENABLED(CONFIG_MITIGATION_ITS) ? ITS_MITIGATION_AUTO :
> > +ITS_MITIGATION_OFF;
>
> Ditto.
>
> >  static int __ro_after_init retbleed_nosmt = false;
> >
> >  static int __init retbleed_parse_cmdline(char *str) @@ -1242,11
> > +1261,19 @@ static void __init retbleed_update_mitigation(void)
> >       /*
> >        * retbleed=stuff is only allowed on Intel.  If stuffing can't be used
> >        * then a different mitigation will be selected below.
> > +      *
> > +      * its=stuff will also attempt to enable stuffing.
> >        */
> > -     if (retbleed_mitigation == RETBLEED_MITIGATION_STUFF) {
> > +     if (retbleed_mitigation == RETBLEED_MITIGATION_STUFF ||
> > +         its_mitigation == ITS_MITIGATION_RETPOLINE_STUFF) {
> >               if (spectre_v2_enabled != SPECTRE_V2_RETPOLINE) {
>
> SPECTRE_V2_EIBRS_RETPOLINE also enables retpoline.
>
> >                       pr_err("WARNING: retbleed=stuff depends on
> > spectre_v2=retpoline\n");
>
> This can be updated to:
>
>                         pr_err("WARNING: retbleed=stuff depends on retpoline\n");
>

Yeah, I noticed that too.  But the existing upstream code (before my re-write) was also only checking spectre_v2_enabled == SPECTRE_V2_RETPOLINE in retbleed_select_mitigation().

So it seems like CDT previously wasn't supported with spectre_v2=eibrs,retpoline.

If we want to change that, I suggest doing it in a separate patch.


> >       }
> >
> > -     if (cmd == ITS_CMD_RSB_STUFF &&
> > -         (!boot_cpu_has(X86_FEATURE_RETPOLINE) ||
> !IS_ENABLED(CONFIG_MITIGATION_CALL_DEPTH_TRACKING))) {
> > +     if (its_mitigation == ITS_MITIGATION_RETPOLINE_STUFF &&
> > +         !IS_ENABLED(CONFIG_MITIGATION_CALL_DEPTH_TRACKING)) {
> >               pr_err("RSB stuff mitigation not supported, using default\n");
> > -             cmd = ITS_CMD_ON;
> > +             its_mitigation = ITS_MITIGATION_ALIGNED_THUNKS;
>
> This and ...
>
> >       }
> >
> > -     switch (cmd) {
> > -     case ITS_CMD_OFF:
> > +     if (its_mitigation == ITS_MITIGATION_VMEXIT_ONLY &&
> > +         !boot_cpu_has_bug(X86_BUG_ITS_NATIVE_ONLY))
> > +             its_mitigation = ITS_MITIGATION_ALIGNED_THUNKS;
>
> ... this are essentially resetting the mitigation to default. This will be more clear if
> you could change the mitigation to ITS_MITIGATION_AUTO here, and at the end
> have:
>
>         if (its_mitigation == ITS_MITIGATION_AUTO)
>                 its_mitigation = ITS_MITIGATION_ALIGNED_THUNKS;

The point of AUTO is really to say that no cmdline option was specified.  This is relevant for my attack vector patches because AUTO means "defer to the enabled attack vectors".

In the attack vector series, AUTO is checked early in the select function and the mitigation will be disabled if needed.  If it is needed, the default mitigation is chosen.

In the current code without attack vectors, AUTO just always means pick the default, but is really a preparatory thing for the attack vector support.

The code you highlighted deals with cases where an explicit cmdline option was specified asking for mitigation, but it can't be done.  Falling back to the default option is fine, but calling it AUTO I think would be confusing because AUTO (in the next patch series) means "defer to the attack vectors".  So I would prefer to leave the code as-is.


> > +
> > +}
> > +
> > +static void __init its_update_mitigation(void) {
> > +     if (!boot_cpu_has_bug(X86_BUG_ITS) || cpu_mitigations_off())
> > +             return;
> > +
> > +     switch (spectre_v2_enabled) {
> > +     case SPECTRE_V2_NONE:
> > +             pr_err("WARNING: Spectre-v2 mitigation is off, disabling
> > + ITS\n");
> >               its_mitigation = ITS_MITIGATION_OFF;
> >               break;
> > -     case ITS_CMD_VMEXIT:
> > -             if (boot_cpu_has_bug(X86_BUG_ITS_NATIVE_ONLY)) {
> > -                     its_mitigation = ITS_MITIGATION_VMEXIT_ONLY;
> > -                     goto out;
> > -             }
> > -             fallthrough;
> > -     case ITS_CMD_ON:
> > -             its_mitigation = ITS_MITIGATION_ALIGNED_THUNKS;
> > -             if (!boot_cpu_has(X86_FEATURE_RETPOLINE))
> > -                     setup_force_cpu_cap(X86_FEATURE_INDIRECT_THUNK_ITS);
> > -             setup_force_cpu_cap(X86_FEATURE_RETHUNK);
> > -             set_return_thunk(its_return_thunk);
> > +     case SPECTRE_V2_RETPOLINE:
>
> Also SPECTRE_V2_EIBRS_RETPOLINE.

See above.

>
> > +             /* Retpoline+CDT mitigates ITS */
> > +             if (retbleed_mitigation == RETBLEED_MITIGATION_STUFF)
>
>
>
> > +                     its_mitigation = ITS_MITIGATION_RETPOLINE_STUFF;
> >               break;
> > -     case ITS_CMD_RSB_STUFF:
> > -             its_mitigation = ITS_MITIGATION_RETPOLINE_STUFF;
> > -             setup_force_cpu_cap(X86_FEATURE_RETHUNK);
> > -             setup_force_cpu_cap(X86_FEATURE_CALL_DEPTH);
> > -             set_return_thunk(call_depth_return_thunk);
> > -             if (retbleed_mitigation == RETBLEED_MITIGATION_NONE) {
> > -                     retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
> > -                     pr_info("Retbleed mitigation updated to stuffing\n");
> > -             }
> > +     case SPECTRE_V2_LFENCE:
> > +     case SPECTRE_V2_EIBRS_LFENCE:
> > +             pr_err("WARNING: ITS mitigation is not compatible with lfence
> mitigation\n");
> > +             its_mitigation = ITS_MITIGATION_OFF;
> > +             break;
> > +     default:
> >               break;
> >       }
> > -out:
> > +
> > +     /*
> > +      * retbleed_update_mitigation() will try to do stuffing if its=stuff.
> > +      * If it can't, such as if spectre_v2!=retpoline, then fall back to
> > +      * aligned thunks.
> > +      */
> > +     if (its_mitigation == ITS_MITIGATION_RETPOLINE_STUFF &&
> > +         retbleed_mitigation != RETBLEED_MITIGATION_STUFF)
> > +             its_mitigation = ITS_MITIGATION_ALIGNED_THUNKS;
>
> The =stuff mitigation depends on retpoline, not really on retbleed.
> Shouldn't this be handled in the switch (spectre_v2_enabled) above?
>
> >       pr_info("%s\n", its_strings[its_mitigation]);  }
> >
> > +static void __init its_apply_mitigation(void) {
> > +     /* its=stuff forces retbleed stuffing and is enabled there. */
>
> Oh, this is why you are depending on retbleed_mitigation above, this part is a bit
> confusing.
>
> Will think about it more later, trying to have a couple of days off.

It is a bit confusing, no argument there.  And why I spent most of the commit log trying to explain it :)

I do prefer this way of handling it though compared to the existing code.  The existing code would *change* retbleed_mitigation in its_select_mitigation() which I think is very confusing.  I believe the rule for these functions should be that xxx_mitigation is only ever modified in the xxx_select_mitigation() and xxx_update_mitigation() functions.

If there's another idea though (or a place where a comment might help), let me know.

Thanks --David Kaplan

Powered by blists - more mailing lists