Message-ID: <227c8442919d44a790370870aaba416c@AcuMS.aculab.com>
Date:   Fri, 20 Dec 2019 10:59:33 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Andy Lutomirski' <luto@...capital.net>
CC:     Brian Gerst <brgerst@...il.com>, Andy Lutomirski <luto@...nel.org>,
        X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Oleg Nesterov <oleg@...hat.com>
Subject: RE: [PATCH] x86: Remove force_iret()

From: Andy Lutomirski
> Sent: 20 December 2019 10:30
> > On Dec 20, 2019, at 6:10 PM, David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Brian Gerst
> >> Sent: 20 December 2019 03:48
> >>> On Thu, Dec 19, 2019 at 8:50 PM Andy Lutomirski <luto@...nel.org> wrote:
> >>>
> >>> On Thu, Dec 19, 2019 at 3:58 AM Brian Gerst <brgerst@...il.com> wrote:
> >>>>
> >>>> force_iret() was originally intended to prevent the return to user mode with
> >>>> the SYSRET or SYSEXIT instructions, in cases where the register state could
> >>>> have been changed to be incompatible with those instructions.
> >>>
> >>> It's more than that.  Before the big syscall rework, we didn't restore
> >>> the caller-saved regs.  See:
> >>>
> >>> commit 21d375b6b34ff511a507de27bf316b3dde6938d9
> >>> Author: Andy Lutomirski <luto@...nel.org>
> >>> Date:   Sun Jan 28 10:38:49 2018 -0800
> >>>
> >>>    x86/entry/64: Remove the SYSCALL64 fast path
> >>>
> >>> So if you changed r12, for example, the change would get lost.
> >>
> >> force_iret() specifically dealt with changes to CS, SS and EFLAGS.
> >> Saving and restoring the extra registers was a different problem
> >> although it affected the same functions like ptrace, signals, and
> >> exec.
> >
> > Is it ever possible for any of the segment registers to refer to the LDT
> > and for another thread to invalidate the entries 'very late' ?
> 
> Not in newer kernels, because the actual LDT is never modified.
> Instead, LDT changes create a whole new LDT and propagate it with an IPI.

Can the IPI be disabled through the SYSRET path?
Once in user space, the IPI will interrupt the process and, I presume, it will
pick up the new LDT on 'return to user'.
But if the IPI happens between the LDT being set and SYSRET it will (presumably)
remain 'pending' until the next system call?
Which could be long enough for one thread to have passed a pointer across giving
an unexpected SEGV (or maybe worse, failing to give an expected one).

> But the IRET path can fail due to changes to the selectors while in the kernel, due to sigreturn or ptrace.  We have delightful selftests
> for this.
> 
> >
> > So even though the values were valid when changed, they are
> > invalid during the 'return to user' sequence.
> >
> > I remember writing a signal handler that 'corrupted' all the
> > segment registers (etc) and fixing the NetBSD kernel to handle
> > all the faults restoring the segment registers and IRET faulting
> > in kernel (IIRC invalid user %SS or %CS).
> > (IRET can also fault in user space, but that is a normal fault.)
> 
> Did you remember to test the #NP case?  Many kernels forgot that this was possible :)

That might have been why I was fixing it.
I certainly tested the cases where loading the user segment registers faulted in kernel
(after loading the user-GS) and where IRET faulted in kernel.
I fixed up the stack in the interrupt entry code to make it all appear to be a fault
in user-space (deleting one of the trap frames).
This also stops repeated faults getting further and further down the kernel stack.

> > Is it actually cheaper to properly validate the segment registers,
> > or take the 'hit' of the slightly slower IRET path and get the cpu
> > to do it for you?
> 
> The validation we’re talking about is for SYSRET, not IRET.  It has its own set of nasty conditions involving EFLAGS, R11, RIP, and RCX.
> Fortunately no segments are involved. The algorithm is, roughly:
> 
> if (okay for SYSRET) {
>   SYSRET (and assume it can’t fail)
> } else {
>   if (need ESPFIX)
>    Horrible hacks;
>   IRET;
> }

Ok, I was part worried you were forcing 'okay for SYSRET' to 1.
 
> And we handle #GP, #SS, #NP and #DF from IRET. And we have selftests for all of this.

Yes, it would be best if IRET only ever faulted in user-space.

> And no one runs the bloody selftests on 32-bit kernels, resulting in truly awful bugs.

I suspect I didn't try hard enough to get FS/GS and FSBASE/GSBASE correctly restored.
(Given that the segment descriptors may not contain the required values.)

> We can’t handle #GP from SYSRET. Thanks, Intel.
> 
> (AMD gets this more right. SYSRET is still a turd, but it can’t fault. Intel handles RIP canonical checks differently from AMD, and SYSRET
> will #GP if RCX is noncanonical.  The result was privilege escalation on basically every OS when this was noticed.)

Yes, all the 'fast system call' instructions were badly thought out.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists