Message-ID:
 <LV3PR12MB9265FDD41382B1271DB0305E94642@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Thu, 12 Sep 2024 19:57:50 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Dave Hansen <dave.hansen@...el.com>, 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" <x86@...nel.org>, "H .
 Peter Anvin" <hpa@...or.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH 27/34] x86/bugs: Add attack vector controls for
 spectre_v1

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@...el.com>
> Sent: Thursday, September 12, 2024 2:37 PM
> To: Kaplan, David <David.Kaplan@....com>; 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>
> Cc: linux-kernel@...r.kernel.org
> Subject: Re: [RFC PATCH 27/34] x86/bugs: Add attack vector controls for
> spectre_v1
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 9/12/24 12:08, David Kaplan wrote:
> > @@ -1114,6 +1114,9 @@ static void __init
> > spectre_v1_select_mitigation(void)
> >  {
> >       if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) ||
> cpu_mitigations_off())
> >               spectre_v1_mitigation = SPECTRE_V1_MITIGATION_NONE;
> > +
> > +     if (!should_mitigate_vuln(SPECTRE_V1))
> > +             spectre_v1_mitigation = SPECTRE_V1_MITIGATION_NONE;
> >  }
>
> Just a high-level comment on this: usually in a well-structured series that has
> sufficient refactoring, if you start to look at the end of the series, things start
> to fall into place.  The series (at some point) stops adding complexity things get
> simpler.
>
> I don't really see that inflection point here.
>
> For instance, I would have expected cpu_mitigations_off() to be consulted in
> should_mitigate_vuln() so that some of the individual sites can go away.

In the existing functionality, mitigations=off overrides everything, even other bug-specific command line options.  While the should_mitigate_vuln() is only called if the mitigation remains as AUTO (meaning no bug-specific command line option was passed).  So moving the cpu_mitigations_off() check into should_mitigate_vuln() would be a functional change to current behavior.

Feedback on that is certainly welcome, I was trying to be cautious about not changing any existing command line behavior or interactions.

>
> There's also added complexity from having 'enum vulnerabilities' which
> basically duplicate the X86_BUG_* space.  If the infrastructure was, for
> instance, built around X86_BUG bits, it might have enabled this patch to be
> something like:
>
> -       if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) ||
> -           cpu_mitigations_off())
> +       if (!should_mitigate_vuln(X86_BUG_SPECTRE_V1))
>                 spectre_v1_mitigation = SPECTRE_V1_MITIGATION_NONE;

That's a reasonable idea.  One issue I see is that there is no separation in the X86_BUG* space for spectre_v2 vs spectre_v2_user, but they do have separate mitigations.  But I think that is the only missing one, so maybe it just makes sense to add a X86_BUG bit for that?

>
> I'm also not sure this series takes the right approach in representing logic in
> data structures versus code.
>
> For instance, this:
>
> > +     case MDS:
> > +     case TAA:
> > +     case MMIO:
> > +     case RFDS:
> > +     case SRBDS:
> > +     case GDS:
> > +             return cpu_mitigate_attack_vector(CPU_MITIGATE_USER_KERNEL)
> ||
> > +                     cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_HOST) ||
> > +                     cpu_mitigate_attack_vector(CPU_MITIGATE_USER_USER) ||
> > +
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_GUEST);
>
> We've _tended_ to represent these in data structure like cpu_vuln_whitelist.
>
> struct whatever var[] =
>    MACRO(MDS,  USER_KERNEL | GUEST_HOST | USER_USER | GUEST_GUEST)
>    MACRO(MMIO, USER_KERNEL | GUEST_HOST | USER_USER |
> GUEST_GUEST)
>    ...
> };

Ah, yeah I could do that.  I think the case statement makes it a bit easier to see groupings of which issues involve the same attack vectors, although that's also covered in the documentation file.

I'm not opposed to using a data structure for this if that’s more consistent with other areas.

>
> But I do like the concept of users being focused on the attack vectors in
> general.  That part is really nice.
>
> As we talk about this at Plumbers, we probably need to be focused on
> whether users want this new attack-vector-based selection mechanism or the
> old style.  Because adding the attack-vector style is going to add complexity
> any way we do it.

And to be clear, I was trying to continue to support both.  But the attack-vector style is also more future-proof because when new issues arise, they would get added to the appropriate vectors and users wouldn't have to do anything ideally.

Thanks
--David Kaplan

Powered by blists - more mailing lists