Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak | From | Andy Lutomirski <> | Date | Mon, 25 Feb 2019 08:29:12 -0800 |
| |
> On Feb 25, 2019, at 8:10 AM, Peter Zijlstra <peterz@infradead.org> 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
| |