linux-kernel - Re: [PATCH v3 04/35] x86/bugs: Restructure mds 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: <CA+i-1C0AV8pDU=bwBShfERAVmUhMpHF9WXjGStb7_5BG-sUkdQ@mail.gmail.com>
Date: Mon, 10 Feb 2025 18:28:24 +0100
From: Brendan Jackman <jackmanb@...gle.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>, 
	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 04/35] x86/bugs: Restructure mds mitigation

On Mon, 10 Feb 2025 at 18:17, Kaplan, David <David.Kaplan@....com> wrote:
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
> > From: Brendan Jackman <jackmanb@...gle.com>
> > Sent: Monday, February 10, 2025 10:14 AM
> > 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>; 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 04/35] x86/bugs: Restructure mds mitigation
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On Wed, 8 Jan 2025 at 21:27, David Kaplan <david.kaplan@....com> wrote:
> > > +/*
> > > + * Speculation Vulnerability Handling
> > > + *
> > > + * Each vulnerability is handled with the following functions:
> > > + *   <vuln>_select_mitigation() -- Selects a mitigation to use.  This should
> > > + *                                take into account all relevant command line
> > > + *                                options.
> > > + *   <vuln>_update_mitigation() -- This is called after all vulnerabilities have
> > > + *                                selected a mitigation, in case the selection
> > > + *                                may want to change based on other choices
> > > + *                                made.  This function is optional.
> > > + *   <vuln>_apply_mitigation() -- Enable the selected mitigation.
> >
> > Maybe also worth calling out cpu_bugs_smt_update() here?
>
> Hmm, how were you thinking?  The 3 functions above are defined for each vulnerability.  So this is more intended as a guide where if adding a new vulnerability, you should define the functions above as needed.

Yeah it's not really needed for people adding new mitigations but it's
still helpful for readers IMO.

Just something like "see also  cpu_bugs_smt_update()" to highlight
that just coz all the *_apply_mitigation()s are done, it doesn't mean
we've finished setting up the mitigations yet.

(Like with the naming bikeshed though, this is very much a nonblocking
suggestion!)

> > > +/* Return TRUE if any VERW-based mitigation is enabled. */ static
> > > +bool __init verw_mitigation_enabled(void) {
> > > +       return (mds_mitigation != MDS_MITIGATION_OFF ||
> > > +               (taa_mitigation != TAA_MITIGATION_OFF &&
> > > +                taa_mitigation != TAA_MITIGATION_TSX_DISABLED) ||
> > > +               mmio_mitigation != MMIO_MITIGATION_OFF ||
> > > +               rfds_mitigation != RFDS_MITIGATION_OFF); }
> >
> > Since you defined such nice terminology above, why not use it here and say
> > verw_mitigation_selected()?
> >
> > (Obviously if the alternative was a respin for this trivial issue alone I would prefer to
> > merge with the current name!)
>
> I do like that name better, I'll use that unless anyone else objects.
>
> >
> > > +static void __init mds_update_mitigation(void) {
> > > +       if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off())
> > > +               return;
> > > +
> > > +       /* If TAA, MMIO, or RFDS are being mitigated, MDS gets mitigated too. */
> > > +       if (verw_mitigation_enabled()) {
> > > +               if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> > > +                       mds_mitigation = MDS_MITIGATION_FULL;
> > > +               else
> > > +                       mds_mitigation = MDS_MITIGATION_VMWERV;
> > > +       }
> >
> > This is changing what the user will see in sysfs. This seems good to me, but it
> > would be worth calling it out in the commit log I think.
>
> Does it?  What is the case you're thinking of where it is different vs tip?

Oh, no it doesn't - I forgot about md_clear_update_mitigation(). This
error is a good justification for this refactoring :)