[uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Eli Cooper]

This patch prevents userspace() from incorrectly restoring FPU registers
after a sigreturn or rt_sigreturn system call, which has already restored
FPU registers to the state prior to the signal handler was invoked.

Fixes FPU state corruption after invoking the signal handler.

Signed-off-by: Eli Cooper <elicooper@gmx.com>
---
 arch/um/os-Linux/skas/process.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index 23025d6..664f184 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -310,6 +310,8 @@ void userspace(struct uml_pt_regs *regs)
 	int err, status, op, pid = userspace_pid[0];
 	/* To prevent races if using_sysemu changes under us.*/
 	int local_using_sysemu;
+	/* To prevent FPU register restore after sigreturn syscalls. */
+	int skip_fp_restore = 0;
 	siginfo_t si;
 
 	/* Handle any immediate reschedules or signals */
@@ -328,8 +330,9 @@ void userspace(struct uml_pt_regs *regs)
 		if (ptrace(PTRACE_SETREGS, pid, 0, regs->gp))
 			fatal_sigsegv();
 
-		if (put_fp_registers(pid, regs->fp))
-			fatal_sigsegv();
+		if (!skip_fp_restore)
+			if (put_fp_registers(pid, regs->fp))
+				fatal_sigsegv();
 
 		/* Now we set local_using_sysemu to be used for one loop */
 		local_using_sysemu = get_using_sysemu();
@@ -351,6 +354,7 @@ void userspace(struct uml_pt_regs *regs)
 		}
 
 		regs->is_user = 1;
+		skip_fp_restore = 0;
 		if (ptrace(PTRACE_GETREGS, pid, 0, regs->gp)) {
 			printk(UM_KERN_ERR "userspace - PTRACE_GETREGS failed, "
 			       "errno = %d\n", errno);
@@ -381,6 +385,12 @@ void userspace(struct uml_pt_regs *regs)
 				else handle_segv(pid, regs);
 				break;
 			case SIGTRAP + 0x80:
+#ifdef __i386__
+				if (PT_SYSCALL_NR(regs->gp) == __NR_sigreturn)
+					skip_fp_restore = 1;
+#endif
+				if (PT_SYSCALL_NR(regs->gp) == __NR_rt_sigreturn)
+					skip_fp_restore = 1;
 			        handle_trap(pid, regs, local_using_sysemu);
 				break;
 			case SIGTRAP:
-- 
2.7.2


------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Richard Weinberger]

Eli,

Am 16.03.2016 um 02:25 schrieb Eli Cooper:
> This patch prevents userspace() from incorrectly restoring FPU registers
> after a sigreturn or rt_sigreturn system call, which has already restored
> FPU registers to the state prior to the signal handler was invoked.
> 
> Fixes FPU state corruption after invoking the signal handler.

First of all, thanks a lot for hunting down these nasty issues!

Where exactly are the FPU regs restored in the sigregturn case?
Not sure if I fully understand the error scenario.

Thanks,
//richard

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Eli Cooper]

Hi Richard,

On 2016/3/18 6:21, Richard Weinberger wrote:
> Where exactly are the FPU regs restored in the sigregturn case?
> Not sure if I fully understand the error scenario.

Well, sys_sigreturn() or sys_rt_sigreturn() calls copy_sc_from_user(),
and the latter copies fpstate, which is the saved FPU state before the
signal handler was invoked, from sigframe and restores it.
That is correct: after returning from the signal handler, the process is
in the same FPU state before it was invoked.

However, userspace() saves the FPU state before a system call and
restores it after. In the sigreturn case, after sys_sigreturn() returns,
which has already made FPU in the right state, userspace() overwrites it
by making it in the state prior to the sigreturn was called (i.e., the
signal handler's state). That leaves the process in question a corrupted
FPU state.

Eli

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Richard Weinberger]

Eli,

Am 18.03.2016 um 02:41 schrieb Eli Cooper:
> Hi Richard,
> 
> On 2016/3/18 6:21, Richard Weinberger wrote:
>> Where exactly are the FPU regs restored in the sigregturn case?
>> Not sure if I fully understand the error scenario.
> 
> Well, sys_sigreturn() or sys_rt_sigreturn() calls copy_sc_from_user(),
> and the latter copies fpstate, which is the saved FPU state before the
> signal handler was invoked, from sigframe and restores it.
> That is correct: after returning from the signal handler, the process is
> in the same FPU state before it was invoked.
> 
> However, userspace() saves the FPU state before a system call and
> restores it after. In the sigreturn case, after sys_sigreturn() returns,
> which has already made FPU in the right state, userspace() overwrites it
> by making it in the state prior to the sigreturn was called (i.e., the
> signal handler's state). That leaves the process in question a corrupted
> FPU state.

Okay. That's what I thought/feared.
I wonder how other architectures handle this case?
Ideally I'd like to avoid as much extra code as possible in userspace().

Thanks,
//richard

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Eli Cooper]

Hi Richard,

On 2016/3/18 16:20, Richard Weinberger wrote:
> I wonder how other architectures handle this case?
> Ideally I'd like to avoid as much extra code as possible in userspace().

Please forgive my ignorance of other architectures. But to the best of
my knowledge, FPU state is not saved and restored around a system call
in x86.

I agree that we should keep userspace() minimal. So what in the first
place is the FPU state save/restore code for? Or, what (except for
sigreturn) could possibly mess the FPU state of the ptrace'd process
without it?

I tried commenting out the FPU restore code in userspace() and tested
with a bunch of kernel modules and userland programs. Nothing bad
happened so far as I can tell.

Thanks,
Eli

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Jeff Dike]

On Sat, Mar 19, 2016 at 12:13:47AM +0800, Eli Cooper wrote:
> I agree that we should keep userspace() minimal. So what in the first
> place is the FPU state save/restore code for? Or, what (except for
> sigreturn) could possibly mess the FPU state of the ptrace'd process
> without it?

My (vague) recollection was that with libc in there, I couldn't be
confident that it wouldn't unexpectedly use FP for something.  So,
saving and restoring FP state was a hedge against that happening.

				Jeff
-- 
Jeff Dike
AddToIt
978-254-0789 (o)
978-394-8986 (c)

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] um: fix FPU register double-restore after sigreturn

[Richard Weinberger]

Jeff, Eli,

Am 18.03.2016 um 17:42 schrieb Jeff Dike:
> On Sat, Mar 19, 2016 at 12:13:47AM +0800, Eli Cooper wrote:
>> I agree that we should keep userspace() minimal. So what in the first
>> place is the FPU state save/restore code for? Or, what (except for
>> sigreturn) could possibly mess the FPU state of the ptrace'd process
>> without it?
> 
> My (vague) recollection was that with libc in there, I couldn't be
> confident that it wouldn't unexpectedly use FP for something.  So,
> saving and restoring FP state was a hedge against that happening.
> 
> 				Jeff
> 

git blame points to:
commit fbfe9c847edf57ac8232aeafb290f272289893a3
Author: Ingo van Lil <inguin@gmx.de>
Date:   Wed Sep 14 16:21:23 2011 -0700

    um: Save FPU registers between task switches

    Some time ago Jeff prepared 42daba316557 ("uml: stop saving process FP
    state") for UML to stop saving the process FP state between task
    switches.  The assumption was that since with SKAS0 every guest process
    runs inside a host process context the host OS will take care of keeping
    the proper FP state.

    Unfortunately this is not true for multi-threaded applications, where
    all guest threads share a single host process context yet all may use
    the FPU on their own.  Although I haven't verified it I suspect things
    to be even worse in SKAS3 mode where all guest processes run inside a
    single host process.

    The patch reintroduces the saving and restoring of the FP context
    between task switches.

    [richard@nod.at: Ingo posted this patch in 2009, sadly it was never applied
    and got lost. Now in 2011 the problem was reported by Gunnar.]

    Signed-off-by: Ingo van Lil <inguin@gmx.de>
    Signed-off-by: Richard Weinberger <richard@nod.at>
    Reported-by: <gunnarlindroth@hotmail.com>
    Tested-by: <gunnarlindroth@hotmail.com>
    Cc: Stanislav Meduna <stano@meduna.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

So, it is not that easy. :=)

Thanks,
//richard

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel