Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Andy Lutomirski]

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

Yes, except:

> 
> ---
> 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;                    \
> })
> 

Drop this part.

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

And this.

Unfortunately, whether we want the | 3 varies by segment. For FS and GS, we definitely donât want it, since 0 is a common and important value, and 3 is a deeply screwed up value.  (3 is legal to *write* to GS, and it sticks, but IRET silently changes it to 0, because the original 386 designers (I assume) confused themselves.

> 
> #define RELOAD_SEG(seg)        {        \
> -    unsigned int pre = GET_SEG(seg);    \
> +    unsigned int pre = (seg);        \
>    unsigned int cur = get_user_seg(seg);    \
> -    pre |= 3;                \
>    if (pre != 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 = 0;
> +    u16 gs, fs, es, ds;
>    void __user *buf;
>    u32 tmp;
> 
> @@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>    current->restart_block.fn = do_no_restart_syscall;
> 
>    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 = GET_SEG(gs);
> +        fs = GET_SEG(fs);
> +        ds = GET_SEG(ds);
> +        es = GET_SEG(es);
> 
>        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 *regs,
>        buf = compat_ptr(tmp);
>    } get_user_catch(err);
> 
> +    /*
> +     * 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 |= fpu__restore_sig(buf, 1);
> 
>    force_iret();

I