[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACYkzJ6K77T8aFNC0oxcq8-huGZSX8spLpvNDiJQbyYsx5HwNg@mail.gmail.com>
Date: Mon, 20 Feb 2023 15:01:47 -0800
From: KP Singh <kpsingh@...nel.org>
To: Borislav Petkov <bp@...en8.de>
Cc: Andrew Cooper <andrew.cooper3@...rix.com>,
Dave Hansen <dave.hansen@...el.com>,
Josh Poimboeuf <jpoimboe@...nel.org>,
linux-kernel@...r.kernel.org, pjt@...gle.com, evn@...gle.com,
tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
x86@...nel.org, hpa@...or.com, peterz@...radead.org,
pawan.kumar.gupta@...ux.intel.com, kim.phillips@....com,
alexandre.chartre@...cle.com, daniel.sneddon@...ux.intel.com,
José Oliveira <joseloliveira11@...il.com>,
Rodrigo Branco <rodrigo@...nelhacking.com>,
Alexandra Sandulescu <aesa@...gle.com>,
Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH RESEND] x86/speculation: Fix user-mode spectre-v2
protection with KERNEL_IBRS
On Mon, Feb 20, 2023 at 1:10 PM Borislav Petkov <bp@...en8.de> wrote:
>
> On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> > "When IBRS or enhanced IBRS is enabled, STIBP is not needed."
> >
> > This is misleading, if not strictly wrong. The IBRS bit being set
> > implies STIBP, which reads differently to "not needed".
> >
> >
> > Now - eIBRS is "set once at start of day" which ends up becoming a
> > global implicit STIBP.
>
> Right.
>
> > I think we're discussing the legacy IBRS case here. i.e. what was
> > retrofitted in microcode for existing parts?
>
> Any IBRS actually. The one which is *not* the automatic, fire'n'forget
> thing.
>
> > The reason why it is "write 1 on each privilege increase, 0 on privilege
> > decrease" is because on some CPUs its an inhibit control, and on some
> > CPUs is a flush (i.e. its actually IBPB).
> >
> > But these same CPUs don't actually have an ability to thread-tag the
> > indirect predictor nicely so STIBP is also horribly expensive under the
> > hood - so much so that we were firmly recommended to clear STIBP/IBRS
> > when going idle so as to reduce the impact on the sibling.
>
> Yap, we do that. And we do the write to 0 for IBRS on exit to
> luserspace, probably for very similar reasons.
>
> > IMO the proper way to do this is to set STIBP uniformly depending on
> > whether you want it in userspace or not, and treat it logically
> > separately to IBRS. It doesn't hurt (any more) to have both bits set.
>
> So we have this thing:
>
> /*
> * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
> * STIBP is not required.
> */
> if (!boot_cpu_has(X86_FEATURE_STIBP) ||
> !smt_possible ||
> spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> return;
>
> What you propose sounds cleaner but would definitely need more massaging
> of this madness code. So I guess we could do only the
> enable-STIBP-when-IBRS-enabled thing first and do more cleanups later.
>
I do like the idea of decoupling, but yeah this is a bit tangled and
abstracted away from the user. The user currently just selects one of
(note the absence of STIBP in the choices here).
SPECTRE_V2_USER_CMD_NONE,
SPECTRE_V2_USER_CMD_AUTO,
SPECTRE_V2_USER_CMD_FORCE,
SPECTRE_V2_USER_CMD_PRCTL,
SPECTRE_V2_USER_CMD_PRCTL_IBPB,
SPECTRE_V2_USER_CMD_SECCOMP,
SPECTRE_V2_USER_CMD_SECCOMP_IBPB,
and the STIBP mode is selected implicitly based on the kernel's choice
of spectre v2 mitigations. I will fix the default case and we can
eventually decouple STIBP from the v2 kernel mitigation choice.
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists