Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1D02221774
MIME-Version: 1.0
In-Reply-To: <20180119014105.GD14136@redhat.com>
References: <20180118134800.711245485@infradead.org> <20180118140152.830682032@infradead.org>
 <20180118163745.t5nmwdr53wjsl7o5@treble> <73a5735a-6a5b-0e0f-1f0b-e7cd955880d2@intel.com>
 <a0308ee6-959a-0a78-9205-8de0a60948ac@redhat.com> <20180118182431.xvmk6kzxpzu43b43@treble>
 <20180118190842.GA14136@redhat.com> <CALCETrU9Xa6pBd-WuQ9S1s32f7Nb=p9eHEAv7oixf+vZa18ETg@mail.gmail.com>
 <20180119014105.GD14136@redhat.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 18 Jan 2018 20:10:52 -0800
Message-ID: <CALCETrWKjCtML_7m3oa7gQW6sM7LQB2A1Fh_kqKkQu6jF0uimg@mail.gmail.com>
Subject: Re: [PATCH 23/35] x86/speculation: Add basic speculation control code
To:     Andrea Arcangeli <aarcange@redhat.com>
Cc:     Andy Lutomirski <luto@kernel.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        David Woodhouse <dwmw2@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Ashok Raj <ashok.raj@intel.com>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        Andi Kleen <ak@linux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Jun Nakajima <jun.nakajima@intel.com>,
        Asit Mallick <asit.k.mallick@intel.com>,
        Jason Baron <jbaron@akamai.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 18, 2018 at 5:41 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> Hello,
>
> On Thu, Jan 18, 2018 at 03:25:25PM -0800, Andy Lutomirski wrote:
>> I read the whitepaper that documented the new MSRs a couple days ago
>> and I'm now completely unable to find it.  If anyone could send the
>> link, that would be great.
>
> I see Andrew posted a link.
>
>> From memory, however, the docs were quite clear that setting leaving
>> IBRS set when entering user mode or guest mode does not guarantee any
>> particular protection unless an additional CPUID bit (the name of
>> which I forget) is set, and that current CPUs will *not* get that bit
>
> My current understanding is that with SPEC_CTRL alone set in cpuid,
> IBRS is meaningful, other bits don't matter.
>
>> set by microcode update.  IOW the protection given is that, if you set
>> IBRS bit zero after entry to kernel mode, you are protected until you
>> re-enter user mode.  When you're in user mode, you're not protected.
>
> If you leave IBRS set while in user mode, userland is protected as
> strong as kernel mode.

The link that Andrew posted says:

If software sets IA32_SPEC_CTRL.IBRS to 1 after a transition to a more
privileged predictor mode, predicted targets of indirect branches
executed in that predictor mode with IA32_SPEC_CTRL.IBRS = 1 cannot be
controlled by software that was executed in a less privileged
predictor mode or on another logical processor.

...

Enabling IBRS does not prevent software from controlling the predicted
targets of indirect branches of unrelated software executed later at
the same predictor mode (for example, between two different user
applications, or two different virtual machines). Such isolation can
be ensured through use of the IBPB command, described in Section
2.5.3, “Indirect Branch Predictor Barrier (IBPB)”.

So maybe it's poorly written, but I see nothing in that language that
suggests that IBRS=1 (on a CPU without enhanced IBRS) provides any
guarantees at all about who can or cannot control speculation of
indirect branches in user mode.

>
> When you return to kernel mode you've to call IBRS again even if it
> was left set, because there's a higher-privilege mode change. That's
> equivalent to calling only IBPB and leaving STIBP set (only way to
> understand the locations where IBRS has to be set is to imagine IBRS
> as a strict "STIBP; IBPB").

Then Intel should improve their spec to say so.

> IBRS Q/A:
>
> 1) When to write IBRS in SPEC_CTRL? -> imagine it as "STIBP; IBPB"
>
> 2) When to leave IBRS set or when to call IBPB -> imagine the previous
> setting of IBRS as temporarily disabling indirect branch prediction
> without an IBPB implicit in IBRS
>
> If you think it only like 1) you risk missing some places where you've
> to write IBRS even if it was already set.
>
> If you think it only like 2) you risk clearing it too early or you
> risk missing a necessary IBPB.
>
> It has to be thought simultaneously in both ways.

From your description, it sounds like what's actually happening is:

When IBRS is enabled, indirect branch speculation targets cannot be
controlled by code that executed on a different logical processor on
by code that executed prior to the most recent time that IBRS was set
to 1.  Additionally, if the CPU supports enhanced IBRS, then indirect
branch speculation targets cannot be controlled by code that executed
at a less privileged predictor mode.

Is that actually correct?  If so, could Intel please *say* so?
Because writing voodoo magic security code seriously sucks.

>
> The sure thing I get from the specs is IBRS always implies STIBP (even
> when STIBP is a noop), specs are pretty explicit about that.

Ah, it says:

As noted in Section 2.5.1, “Indirect Branch Restricted Speculation
(IBRS)”, enabling IBRS prevents software operating on one logical
processor from controlling the predicted targets of indirect branches
executed on another logical processor. For that reason, it is not
necessary to enable STIBP when IBRS is  enabled.

So I guess we can write 1 when we enter the kernel, but we probably
want to write 2 instead of 0 when we exit.