| 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
| ||
|
Message-ID: <CAATStaOTMrdserLepxkSdFErrjhMKyvd_g_GNBqOo_4p932ikw@mail.gmail.com>
Date: Mon, 2 Nov 2020 11:02:10 +1100
From: "Anand K. Mistry" <amistry@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: x86@...nel.org, Joel Fernandes <joelaf@...gle.com>,
Anthony Steinhauser <asteinhauser@...gle.com>,
tglx@...utronix.de, Borislav Petkov <bp@...en8.de>,
"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Mark Gross <mgross@...ux.intel.com>,
Mike Rapoport <rppt@...nel.org>,
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
Tony Luck <tony.luck@...el.com>,
Vineela Tummalapalli <vineela.tummalapalli@...el.com>,
Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally
enabled on CPUs with always-on STIBP
On Sun, 1 Nov 2020 at 02:05, Tom Lendacky <thomas.lendacky@....com> wrote:
>
> On 10/29/20 1:51 AM, Anand K Mistry wrote:
> > On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON,
> > STIBP is set to on and 'spectre_v2_user_stibp ==
> > SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to
> > conditional. However, this leads to the case where it's impossible to
> > turn on IBPB for a process because in the PR_SPEC_DISABLE case in
> > ib_prctl_set, the (spectre_v2_user_stibp ==
> > SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the
> > task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE
> > even though IBPB is set to conditional.
> >
> > More generally, the following cases are possible:
> > 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb
> > 2. STIBP = on && IBPB = conditional for AMD CPUs with
> > X86_FEATURE_AMD_STIBP_ALWAYS_ON
> >
> > The first case functions correctly today, but only because
> > spectre_v2_user_ibpb isn't updated to reflect the IBPB mode.
> >
> > At a high level, this change does one thing. If either STIBP is IBPB is
>
> s/STIBP is IBPB/STIBP or IBPB/
Oops. Will be fixed in v2.
>
> > set to conditional, allow the prctl to change the task flag. Also,
> > reflect that capability when querying the state. This isn't perfect
> > since it doesn't take into account if only STIBP or IBPB is
> > unconditionally on. But it allows the conditional feature to work as
> > expected, without affecting the unconditional one.
> >
> > Signed-off-by: Anand K Mistry <amistry@...gle.com>
> >
> > ---
> >
> > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++-----------------
> > 1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index d3f0db463f96..fb64e02eed6f 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
> > return 0;
> > }
> >
> > +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode)
>
> Maybe something like is_spec_ib_user_controlled() would be a better name.
Changed in v2.
>
> > +{
> > + return mode == SPECTRE_V2_USER_PRCTL || mode == SPECTRE_V2_USER_SECCOMP;
> > +}
> > +
>
> I like the idea of passing in the mode you want to check, but it appears
> they are never used independently. The ibpb and stibp modes are always
> checked together in one of the if statements below, so you could make this
> a function that checks both modes and just have a single call. I'll leave
> that up to the maintainers to see what is preferred.
I can see both sides to this. Personally, I think I prefer it as-is
since I think it improves readability a bit by making the conditions
less complicated whilst not hiding too many details. I'll wait to see
what others say before changing this one.
>
> > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
> > {
> > switch (ctrl) {
> > @@ -1262,13 +1267,16 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
> > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE)
> > return 0;
> > /*
> > - * Indirect branch speculation is always disabled in strict
> > - * mode. It can neither be enabled if it was force-disabled
> > - * by a previous prctl call.
> > + * With strict mode for both IBPB and STIBP, the instruction
> > + * code paths avoid checking this task flag and instead,
> > + * unconditionally run the instruction. However, STIBP and IBPB
> > + * are independent and either can be set to conditionally
> > + * enabled regardless of the mode of the other. If either is set
> > + * to conditional, allow the task flag to be updated, unless it
> > + * was force-disabled by a previous prctl call.
>
> You probably want to reference the STIBP always on mode that allows this
> situation.
Updated comment in v2 to mention the X86_FEATURE_AMD_STIBP_ALWAYS_ON case.
>
> > */
> > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED ||
> > + if ((!is_spec_ib_user(spectre_v2_user_ibpb) &&
> > + !is_spec_ib_user(spectre_v2_user_stibp)) ||
> > task_spec_ib_force_disable(task))
> > return -EPERM;
> > task_clear_spec_ib_disable(task);
> > @@ -1283,9 +1291,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
> > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE &&
> > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE)
> > return -EPERM;
> > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED)
> > + if (!is_spec_ib_user(spectre_v2_user_ibpb) &&
> > + !is_spec_ib_user(spectre_v2_user_stibp))
>
> The set function seems reasonable to me.
>
> > return 0;
> > task_set_spec_ib_disable(task);
> > if (ctrl == PR_SPEC_FORCE_DISABLE)
> > @@ -1351,20 +1358,18 @@ static int ib_prctl_get(struct task_struct *task)
> > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE &&
> > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE)
> > return PR_SPEC_ENABLE;
> > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED)
> > - return PR_SPEC_DISABLE;
> > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL ||
> > - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL ||
> > - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) {
> > + else if (is_spec_ib_user(spectre_v2_user_ibpb) ||
> > + is_spec_ib_user(spectre_v2_user_stibp)) {
> > if (task_spec_ib_force_disable(task))
> > return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE;
> > if (task_spec_ib_disable(task))
> > return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
> > return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
> > - } else
> > + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT ||
> > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT ||
> > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED)
> > + return PR_SPEC_DISABLE;
> > + else
>
> The get function also seems reasonable.
>
> Lets hear what some of the other folks that are familiar with this area
> have to say.
>
> Thanks,
> Tom
>
> > return PR_SPEC_NOT_AFFECTED;
> > }
> >
> >
--
Anand K. Mistry
Software Engineer
Google Australia
Powered by blists - more mailing lists