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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <alpine.DEB.2.21.1904050007050.1802@nanos.tec.linutronix.de>
Date:   Fri, 5 Apr 2019 07:50:26 -0600
Cc:     "Chang S. Bae" <chang.seok.bae@intel.com>,
        Ingo Molnar <mingo@kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        "H . Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>,
        Ravi Shankar <ravi.v.shankar@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <5DCF2089-98EC-42D3-96C3-6ECCDA0B18E2@amacapital.net>
References: <1552680405-5265-1-git-send-email-chang.seok.bae@intel.com> <1552680405-5265-9-git-send-email-chang.seok.bae@intel.com> <alpine.DEB.2.21.1903251003090.1798@nanos.tec.linutronix.de> <alpine.DEB.2.21.1904050007050.1802@nanos.tec.linutronix.de>
To:     Thomas Gleixner <tglx@linutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Apr 5, 2019, at 2:35 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>=20
>> On Mon, 25 Mar 2019, Thomas Gleixner wrote:
>>> On Fri, 15 Mar 2019, Chang S. Bae wrote:
>>> ENTRY(paranoid_exit)
>>>    UNWIND_HINT_REGS
>>>    DISABLE_INTERRUPTS(CLBR_ANY)
>>>    TRACE_IRQS_OFF_DEBUG
>>> +    ALTERNATIVE "jmp .Lparanoid_exit_no_fsgsbase",    "nop",\
>>> +        X86_FEATURE_FSGSBASE
>>> +    wrgsbase    %rbx
>>> +    jmp    .Lparanoid_exit_no_swapgs;
>>=20
>> Again. A few newlines would make it more readable.
>>=20
>> This modifies the semantics of paranoid_entry and paranoid_exit. Looking a=
t
>> the usage sites there is the following code in the nmi maze:
>>=20
>>    /*
>>     * Use paranoid_entry to handle SWAPGS, but no need to use paranoid_ex=
it
>>     * as we should not be calling schedule in NMI context.
>>     * Even with normal interrupts enabled. An NMI should not be
>>     * setting NEED_RESCHED or anything that normal interrupts and
>>     * exceptions might do.
>>     */
>>    call    paranoid_entry
>>    UNWIND_HINT_REGS
>>=20
>>    /* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */
>>    movq    %rsp, %rdi
>>    movq    $-1, %rsi
>>    call    do_nmi
>>=20
>>    /* Always restore stashed CR3 value (see paranoid_entry) */
>>    RESTORE_CR3 scratch_reg=3D%r15 save_reg=3D%r14
>>=20
>>    testl    %ebx, %ebx            /* swapgs needed? */
>>    jnz    nmi_restore
>> nmi_swapgs:
>>    SWAPGS_UNSAFE_STACK
>> nmi_restore:
>>    POP_REGS
>>=20
>> I might be missing something, but how is that supposed to work when
>> paranoid_entry uses FSGSBASE? I think it's broken, but if it's not then
>> there is a big fat comment missing explaining why.
>=20
> So this _is_ broken.
>=20
>   On entry:
>=20
>      rbx =3D rdgsbase()
>      wrgsbase(KERNEL_GS)
>=20
>   On exit:
>=20
>      if (ebx =3D=3D 0)
>           swapgs
>=20
> The resulting matrix:
>=20
>   |  ENTRY GS    | RBX        | EXIT        | GS on IRET    | RESULT
>   |        |        |        |        |
> 1 |  KERNEL_GS    | KERNEL_GS    | EBX =3D=3D 0    | USER_GS    | FAIL
>   |        |        |        |        |
> 2 |  KERNEL_GS    | KERNEL_GS    | EBX !=3D 0    | KERNEL_GS    | ok
>   |        |        |        |        |
> 3 |  USER_GS    | USER_GS    | EBX =3D=3D 0    | USER_GS    | ok
>   |        |        |        |        |
> 4 |  USER_GS    | USER_GS    | EBX !=3D 0    | KERNEL_GS    | FAIL
>=20
>=20
> #1 Just works by chance because it's unlikely that the lower 32bits of a
>   per CPU kernel GS are all 0.
>=20
>   But it's just a question of probability that this turns into a
>   non-debuggable once per year crash (think KASLR).
>=20
> #4 This can happen when the NMI hits the kernel in some other entry code
>   _BEFORE_ or _AFTER_ swapgs.
>=20
>   User space using GS addressing with GS[31:0] !=3D 0 will crash and burn.=

>=20
>  =20

Hi all-

In a previous incarnation of these patches, I complained about the use of SW=
APGS in the paranoid path. Now I=E2=80=99m putting my maintainer foot down. =
 On a non-FSGSBASE system, the paranoid path known, definitively, which GS i=
s where, so SWAPGS is annoying. With FSGSBASE, unless you start looking at t=
he RIP that you interrupted, you cannot know whether you have user or kernel=
 GSBASE live, since they can have literally the same value.  One of the nume=
rous versions of this patch compared the values and just said =E2=80=9Cwell,=
 it=E2=80=99s harmless to SWAPGS if user code happens to use the same value a=
s the kernel=E2=80=9D.  I complained that it was far too fragile.

So I=E2=80=99m putting my foot down. If you all want my ack, you=E2=80=99re g=
oing to save the old GS, load the new one with WRGSBASE, and, on return, you=
=E2=80=99re going to restore the old one with WRGSBASE. You will not use SWA=
PGS in the paranoid path.

Obviously, for the non-paranoid path, it all keeps working exactly like it d=
oes now.

Furthermore, if you folks even want me to review this series, the ptrace tes=
ts need to be in place.  On inspection of the current code (after the debacl=
e a few releases back), it appears the SETREGSET=E2=80=99s effect depends on=
 the current values in the registers =E2=80=94 it does not actually seem to r=
eliably load the whole state. So my confidence will be greatly increased if y=
our series first adds a test that detects that bug (and fails!), then fixes t=
he bug in a tiny little patch, then adds FSGSBASE, and keeps the test workin=
g.

=E2=80=94Andy=