linux-kernel - RE: [PATCH v3 05/35] x86/bugs: Restructure taa mitigation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <LV3PR12MB9265E7765694B3755E924F8494FD2@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Tue, 11 Feb 2025 17:17:15 +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 05/35] x86/bugs: Restructure taa mitigation

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@...nel.org>
> Sent: Monday, February 10, 2025 4:50 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 05/35] x86/bugs: Restructure taa mitigation
>
> 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:24:45PM -0600, David Kaplan wrote:
> > @@ -400,48 +402,71 @@ static void __init taa_select_mitigation(void)
> >               return;
> >       }
> >
> > -     if (cpu_mitigations_off()) {
> > +     if (cpu_mitigations_off())
> >               taa_mitigation = TAA_MITIGATION_OFF;
> > -             return;
> > -     }
> >
> >       /*
> >        * TAA mitigation via VERW is turned off if both
> >        * tsx_async_abort=off and mds=off are specified.
> > +      *
> > +      * MDS mitigation will be checked in taa_update_mitigation().
> >        */
> > -     if (taa_mitigation == TAA_MITIGATION_OFF &&
> > -         mds_mitigation == MDS_MITIGATION_OFF)
> > +     if (taa_mitigation == TAA_MITIGATION_OFF)
> >               return;
>
> This check seems rather pointless, the only thing after this is the
> TAA_MITIGATION_AUTO check.

True, and it can be removed.  But in patch 4 in the mds logic you did suggest having an explicit return to make it clear that none of the later conditions applied.  I'm not sure I feel strongly either way, but I'd like to be consistent.

>
> >
> > -     if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> > +     /* Microcode will be checked in taa_update_mitigation(). */
> > +     if (taa_mitigation == TAA_MITIGATION_AUTO)
> >               taa_mitigation = TAA_MITIGATION_VERW;
> > -     else
> > -             taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
>
> In the previous patch, MDS checks for ucode in both select and update, which is
> overkill.  That should probably be done only in
> mds_update_mitigation() to be consistent with how TAA does it here?

I assume you're referring to the X86_FEATURE_MD_CLEAR check.  I'll fix this in the mds patch to be similar, good idea.

>
> >
> > -     /*
> > -      * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and
> MDS_NO=1.
> > -      * A microcode update fixes this behavior to clear CPU buffers. It also
> > -      * adds support for MSR_IA32_TSX_CTRL which is enumerated by the
> > -      * ARCH_CAP_TSX_CTRL_MSR bit.
> > -      *
> > -      * On MDS_NO=1 CPUs if ARCH_CAP_TSX_CTRL_MSR is not set,
> microcode
> > -      * update is required.
> > -      */
> > -     if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
> > -         !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
> > -             taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> > +}
>
> Extra whitespace here at the end of the function.

Ack

>
> > +static void __init taa_update_mitigation(void) {
> > +     if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off())
> > +             return;
> > +
> > +     if (verw_mitigation_enabled())
> > +             taa_mitigation = TAA_MITIGATION_VERW;
>
> This overwrites TAA_MITIGATION_TSX_DISABLED?
>
> I think reporting TSX disabled here is more accurate than reporting VERW, since
> the VERW is only done to mitigate the other vulns.

Agreed, will fix.

>
> > +static void __init taa_apply_mitigation(void) {
> > +     if (taa_mitigation == TAA_MITIGATION_VERW ||
> > +         taa_mitigation == TAA_MITIGATION_UCODE_NEEDED) {
> > +             /*
> > +              * TSX is enabled, select alternate mitigation for TAA which is
> > +              * the same as MDS. Enable MDS static branch to clear CPU buffers.
> > +              *
> > +              * For guests that can't determine whether the correct microcode is
> > +              * present on host, enable the mitigation for UCODE_NEEDED as well.
> > +              */
> > +             setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> > +
> > +             if (taa_nosmt || cpu_mitigations_auto_nosmt())
> > +                     cpu_smt_disable(false);
> > +     }
> >
> > -     if (taa_nosmt || cpu_mitigations_auto_nosmt())
> > -             cpu_smt_disable(false);
> >  }
>
> Another extra whitespace here at the end of the function.

Ack

Thanks --David Kaplan