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: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20190225161003.GL32477@hirez.programming.kicks-ass.net>
Date:   Mon, 25 Feb 2019 08:29:12 -0800
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Julien Thierry <julien.thierry@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Ingo Molnar <mingo@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        James Morse <james.morse@arm.com>, valentin.schneider@arm.com,
        Brian Gerst <brgerst@gmail.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Andrew Lutomirski <luto@kernel.org>,
        Borislav Petkov <bp@alien8.de>,
        Denys Vlasenko <dvlasenk@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <3F83AE74-3F9E-42BB-8F9C-3033F72B3EF5@amacapital.net>
References: <20190225124330.613028745@infradead.org> <20190225125231.936952143@infradead.org> <CALCETrW-p4iF-RMecyNt1JDwKh74c_tw67FCW7Yr_Fd3adtGUw@mail.gmail.com> <20190225161003.GL32477@hirez.programming.kicks-ass.net>
To:     Peter Zijlstra <peterz@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Feb 25, 2019, at 8:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
>> On Mon, Feb 25, 2019 at 07:41:50AM -0800, Andy Lutomirski wrote:
>> This is so tangled.
>>=20
>> How about changing RELOAD_SEG to replace unsigned int pre =3D
>> GET_SEG(seg); with unsigned int pre =3D (seg); to make it less magic.
>> Then do:
>>=20
>> unsigned int gs =3D GET_SEG(gs);
>>=20
>> ...
>>=20
>> RELOAD_SEG(gs);
>>=20
>> And now the code actually does what it looks like it does.
>=20
> Is this what you mean?

Yes, except:

>=20
> ---
> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> index 321fe5f5d0e9..e04eeeddcc35 100644
> --- a/arch/x86/ia32/ia32_signal.c
> +++ b/arch/x86/ia32/ia32_signal.c
> @@ -53,17 +53,16 @@
> #define GET_SEG(seg)        ({            \
>    unsigned short tmp;                \
>    get_user_ex(tmp, &sc->seg);            \
> -    tmp;                        \
> +    tmp | 3;                    \
> })
>=20

Drop this part.

> #define COPY_SEG_CPL3(seg)    do {            \
> -    regs->seg =3D GET_SEG(seg) | 3;            \
> +    regs->seg =3D GET_SEG(seg);            \
> } while (0)

And this.

Unfortunately, whether we want the | 3 varies by segment. For FS and GS, we d=
efinitely don=E2=80=99t want it, since 0 is a common and important value, an=
d 3 is a deeply screwed up value.  (3 is legal to *write* to GS, and it stic=
ks, but IRET silently changes it to 0, because the original 386 designers (I=
 assume) confused themselves.

>=20
> #define RELOAD_SEG(seg)        {        \
> -    unsigned int pre =3D GET_SEG(seg);    \
> +    unsigned int pre =3D (seg);        \
>    unsigned int cur =3D get_user_seg(seg);    \
> -    pre |=3D 3;                \
>    if (pre !=3D cur)                \
>        set_user_seg(seg, pre);        \
> }
> @@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,=

>                   struct sigcontext_32 __user *sc)
> {
>    unsigned int tmpflags, err =3D 0;
> +    u16 gs, fs, es, ds;
>    void __user *buf;
>    u32 tmp;
>=20
> @@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *reg=
s,
>    current->restart_block.fn =3D do_no_restart_syscall;
>=20
>    get_user_try {
> -        /*
> -         * Reload fs and gs if they have changed in the signal
> -         * handler.  This does not handle long fs/gs base changes in
> -         * the handler, but does not clobber them at least in the
> -         * normal case.
> -         */
> -        RELOAD_SEG(gs);
> -        RELOAD_SEG(fs);
> -        RELOAD_SEG(ds);
> -        RELOAD_SEG(es);
> +        gs =3D GET_SEG(gs);
> +        fs =3D GET_SEG(fs);
> +        ds =3D GET_SEG(ds);
> +        es =3D GET_SEG(es);
>=20
>        COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
>        COPY(dx); COPY(cx); COPY(ip); COPY(ax);
> @@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *re=
gs,
>        buf =3D compat_ptr(tmp);
>    } get_user_catch(err);
>=20
> +    /*
> +     * Reload fs and gs if they have changed in the signal
> +     * handler.  This does not handle long fs/gs base changes in
> +     * the handler, but does not clobber them at least in the
> +     * normal case.
> +     */
> +    RELOAD_SEG(gs);
> +    RELOAD_SEG(fs);
> +    RELOAD_SEG(ds);
> +    RELOAD_SEG(es);
> +
>    err |=3D fpu__restore_sig(buf, 1);
>=20
>    force_iret();

I=