linux-kernel - RE: [PATCH v2 11/35] x86/bugs: Restructure spectre

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:
 <LV3PR12MB9265442FE51C215C7AFE7B75945B2@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Thu, 14 Nov 2024 17:48:37 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
CC: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>,
	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 v2 11/35] x86/bugs: Restructure spectre_v1 mitigation

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> Sent: Thursday, November 14, 2024 11:41 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>; 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 v2 11/35] x86/bugs: Restructure spectre_v1 mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, Nov 14, 2024 at 03:36:44PM +0000, Kaplan, David wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > > -----Original Message-----
> > > From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> > > Sent: Thursday, November 14, 2024 12:57 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>; 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 v2 11/35] x86/bugs: Restructure spectre_v1
> > > mitigation
> > >
> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
> > >
> > >
> > > On Tue, Nov 05, 2024 at 03:54:31PM -0600, David Kaplan wrote:
> > > >  static void __init spectre_v1_select_mitigation(void)
> > > >  {
> > > > -     if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) ||
> cpu_mitigations_off()) {
> > > > +     if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) ||
> > > > + cpu_mitigations_off())
> > > >               spectre_v1_mitigation = SPECTRE_V1_MITIGATION_NONE;
> > > > +}
> > > > +
> > > > +static void __init spectre_v1_apply_mitigation(void) {
> > > > +     if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) ||
> > > > +cpu_mitigations_off())
> > >
> > > We probably don't need to repeat this check, is this okay:
> > >
> > >         if (spectre_v1_mitigation == SPECTRE_V1_MITIGATION_NONE)
> > > >               return;
> > > > -     }
> > > >
> > > >       if (spectre_v1_mitigation == SPECTRE_V1_MITIGATION_AUTO) {
> >
> > I don't think so.  That would stop us from printing the message about
> > the system being vulnerable at the end of the function.
>
> Sorry it wasn't clear, my comment was not about the return, but about simplifying
> the check:
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index
> 22fdaaac2d21..e8c481c7a590 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1115,7 +1115,7 @@ static void __init spectre_v1_select_mitigation(void)
>
>  static void __init spectre_v1_apply_mitigation(void)  {
> -       if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1) || cpu_mitigations_off())
> +       if (spectre_v1_mitigation == SPECTRE_V1_MITIGATION_NONE)
>                 return;
>
>         if (spectre_v1_mitigation == SPECTRE_V1_MITIGATION_AUTO) {
>
> Since we already set spectre_v1_mitigation to
> SPECTRE_V1_MITIGATION_NONE for that exact condition.

Right, but this gets to my point that this changes whether we issue the print statement or not.  In the current upstream code, it will print a message saying the CPU is vulnerable if 'nospectre_v1' is passed, but not print the message if mitigations=off is passed.

So in the patch, it was keeping the same behavior.

However as noted in my latest reply, there is a lot of inconsistency in bugs.c about behavior here and whether a message is printed when mitigations are disabled in various ways.  I think we need to resolve that, and then I can make it consistent.  If the decision is to not print messages if the bug is explicitly disabled (either via the global or bug-specific option), then I agree with your diff.

--David Kaplan