| Date | Thu, 6 Jun 2019 15:09:13 +0200 | From | Pavel Machek <> | Subject | Re: [PATCH 4.19 208/276] x86/ia32: Fix ia32_restore_sigcontext() AC leak |
| |
Hi!
(stable removed from cc).
> [ Upstream commit 67a0514afdbb8b2fc70b771b8c77661a9cb9d3a9 ] > > Objtool spotted that we call native_load_gs_index() with AC set. > Re-arrange the code to avoid that.
Does this introduce undefined behaviour? > > @@ -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);
es is unitialized at this point, and we can trap.
> + 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); > +
But now we use uninitialized value in es...
> err |= fpu__restore_sig(buf, 1); > > force_iret();
Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [unhandled content-type:application/pgp-signature] |