Message-ID: <20250519235101.2vm6sc5txyoykb2r@desk>
Date: Mon, 19 May 2025 16:51:01 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
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" <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

On Fri, May 16, 2025 at 03:06:26PM +0000, Kaplan, David wrote:
> > > +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.

Thats because there was no need for CDT when eIBRS was enabled, until ITS.
The current upstream ITS mitigation behavior is to allow CDT with eIBRS.
This restructuring is changing that as ITS now relies on retbleed
mitigation, but retbleed_update_mitigation() is refusing to enable CDT when
eIBRS is enabled.

> 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.

Okay, I see your point. But it is debatable whether not selecting a
mitigation (which is auto) is equivalent to selecting an invalid mitigation
which then falls back to the default (also equivalent to auto).

> 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.

In the other series can we make this subtlity more clear with:

enum its_mitigation {
        ITS_MITIGATION_OFF,
        ITS_MITIGATION_AUTO,
        ITS_MITIGATION_VECTOR_BASED = ITS_MITIGATION_AUTO,
	...
};

> > > +
> > > +}
> > > +
> > > +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.

One way to handle intersection between two mitigations is via a common
function that each mitigation can call. Something on the lines of
set_return_thunk().

Powered by blists - more mailing lists