RE: [PATCH v3 21/35] x86/bugs: Determine relevant vulnerabilities based on attack vector controls.

Kaplan, David
2025-02-11T18:55:14Z

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@kernel.org>
> Sent: Tuesday, February 11, 2025 12:42 PM
> To: Kaplan, David <David.Kaplan@amd.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>; Borislav Petkov <bp@alien8.de>; Peter
> Zijlstra <peterz@infradead.org>; Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com>; Ingo Molnar <mingo@redhat.com>; Dave
> Hansen <dave.hansen@linux.intel.com>; x86@kernel.org; H . Peter Anvin
> <hpa@zytor.com>; linux-kernel@vger.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