Message-ID:
 <LV3PR12MB9265450D7A7EEE6D2C8DAAB894FD2@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Tue, 11 Feb 2025 18:55:09 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
CC: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.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>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 21/35] x86/bugs: Determine relevant vulnerabilities
 based on attack vector controls.

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@...nel.org>
> Sent: Tuesday, February 11, 2025 12:42 PM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Borislav Petkov <bp@...en8.de>; Peter
> Zijlstra <peterz@...radead.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 21/35] x86/bugs: Determine relevant vulnerabilities based
> on attack vector controls.
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Jan 08, 2025 at 02:25:01PM -0600, David Kaplan wrote:
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 88eba8e4c7fb..175dbbf9b06e 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -347,6 +347,75 @@ static void x86_amd_ssb_disable(void)
> >               wrmsrl(MSR_AMD64_LS_CFG, msrval);  }
> >
> > +/*
> > + * Returns true if vulnerability should be mitigated based on the
> > + * selected attack vector controls
>
> This needs a period.

Ack

>
> > + *
> > + * See Documentation/admin-guide/hw-vuln/attack_vector_controls.rst
> > + */
> > +static bool __init should_mitigate_vuln(unsigned int bug) {
> > +     switch (bug) {
> > +     /*
> > +      * The only spectre_v1 mitigations in the kernel are related to
> > +      * SWAPGS protection on kernel entry.  Therefore, protection is
> > +      * only required for the user->kernel attack vector.
> > +      */
>
> This comment isn't quite correct, there are things like
> array_index_nospec() and barrier_nospec() being used, but those aren't being
> controlled by bugs.c.  They should at least be mentioned here.

Ack, it's really about the controllable mitigations, the *_nospec() functions are done unconditionally.  Will fix.

>
> > +     case X86_BUG_SPECTRE_V1:
> > +             return
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_USER_KERNEL);
> > +
> > +     /*
> > +      * Both spectre_v2 and srso may allow user->kernel or
> > +      * guest->host attacks through branch predictor manipulation.
> > +      */
>
> I don't think this comment adds anything, the code already makes this obvious.

Ok.

>
> > +     case X86_BUG_SPECTRE_V2:
> > +     case X86_BUG_SRSO:
> > +             return cpu_mitigate_attack_vector(CPU_MITIGATE_USER_KERNEL)
> ||
> > +
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_HOST);
>
> This needs aligned:
>
>                 return cpu_mitigate_attack_vector(CPU_MITIGATE_USER_KERNEL) ||
>                        cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_HOST);
>
> Also, aren't cross-thread attacks possible here, thus the need for STIBP?  More
> questions about the cross-thread "vector" below, at the bottom.
>
> > +     /*
> > +      * spectre_v2_user refers to user->user or guest->guest branch
> > +      * predictor attacks only.  Other indirect branch predictor attacks
> > +      * are covered by the spectre_v2 vulnerability.
> > +      */
>
> The code is already self-evident IMO, I don't think the comment adds anything.

I'll beg to differ on this one, when I started looking at the code I didn't find it obvious that spectre_v2 only referred to the user->kernel and guest->host vectors while spectre_v2_user handles the others.  Additionally I don't know it's immediately obvious from the name that spectre_v2_user handles guest->guest.

>
> > +     case X86_BUG_SPECTRE_V2_USER:
> > +             return cpu_mitigate_attack_vector(CPU_MITIGATE_USER_USER) ||
> > +
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_GUEST);
>
> Another alignment issue.
>
> > +
> > +     /* L1TF is only possible as a guest->host attack */
>
> That's not quite correct, PTE inversion is also done to protect against the user-
> >kernel vector.

Yes good point, I'll fix this.

>
> Also, IIRC the full l1tf mitigation requires disabling SMT, does that not qualify as
> CPU_MITIGATE_CROSS_THREAD?

It does, but this is handled in l1tf_select_mitigation.

>
> > +     case X86_BUG_L1TF:
> > +             return
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_GUEST_HOST);
> > +
> > +     /*
> > +      * All the vulnerabilities below allow potentially leaking data
> > +      * across address spaces.  Therefore, mitigation is required for
> > +      * any of these 4 attack vectors.
> > +      */
> > +     case X86_BUG_MDS:
> > +     case X86_BUG_TAA:
> > +     case X86_BUG_MMIO_STALE_DATA:
> > +     case X86_BUG_RFDS:
> > +     case X86_BUG_SRBDS:
> > +             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);
>
> Some of these also require disabling SMT for their complete mitigations?

Yes, these are handled in their respective functions.

>
> > +     /*
> > +      * GDS can potentially leak data across address spaces and
> > +      * threads.  Mitigation is required under all attack vectors.
> > +      */
> > +     case X86_BUG_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) ||
> > +
> > + cpu_mitigate_attack_vector(CPU_MITIGATE_CROSS_THREAD);
>
> I'm confused by CPU_MITIGATE_CROSS_THREAD here, as the GDS mitigation
> doesn't seem to disable SMT?
>
> Am I just completely misunderstanding the meaning of
> CPU_MITIGATE_CROSS_THREAD?
>
> I assumed it's not a vector per se, but rather it means to force nosmt if one of the
> other enabled mitigations requires doing so for its "full"
> mitigation.  But the implementation doesn't seem to match that.
>
> On the other hand if it really is considered to be its own vector, that doesn't make
> sense either, as "cross-thread attack" is really a subset of each of the other
> vectors.  For example, a user->kernel attack can often be done either via syscall/irq
> or via cross-thread.
>
> So I'm really confused.  Am I missing something?
>

I'll reply to your newer mail.

--David Kaplan

Powered by blists - more mailing lists