[RFC,UKL,07/10] x86/signal: Adjust signal handler register values and return frame

Commit Message

Ali Raza Oct. 3, 2022, 10:21 p.m. UTC

For a UKL thread, returning to a signal handler is not done with iret or
sysret.  This means we need to adjust the way the return stack frame is
handled for these threads.  When constructing the signal frame, we leave
the previous frame in place because we will return to it from the signal
handler.  We also leave space for pushing eflags and the return address.
UKL threads will only use the __KERNEL_DS value in the ss register and 0xC3
in the cs register.

Cc: Jonathan Corbet <[e-mail]>
Cc: Masahiro Yamada <[e-mail]>
Cc: Michal Marek <[e-mail]>
Cc: Nick Desaulniers <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: Dave Hansen <[e-mail]>
Cc: "H. Peter Anvin" <[e-mail]>
Cc: Andy Lutomirski <[e-mail]>
Cc: Eric Biederman <[e-mail]>
Cc: Kees Cook <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Alexander Viro <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Juri Lelli <[e-mail]>
Cc: Vincent Guittot <[e-mail]>
Cc: Dietmar Eggemann <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Ben Segall <[e-mail]>
Cc: Mel Gorman <[e-mail]>
Cc: Daniel Bristot de Oliveira <[e-mail]>
Cc: Valentin Schneider <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Josh Poimboeuf <[e-mail]>

Co-developed-by: Eric B Munson <[e-mail]>
Signed-off-by: Eric B Munson <[e-mail]>
Co-developed-by: Ali Raza <[e-mail]>
Signed-off-by: Ali Raza <[e-mail]>
---
 arch/x86/kernel/signal.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Andy Lutomirski Oct. 4, 2022, 5:34 p.m. UTC | #1

On Mon, Oct 3, 2022, at 3:21 PM, Ali Raza wrote:
> For a UKL thread, returning to a signal handler is not done with iret or
> sysret.  This means we need to adjust the way the return stack frame is
> handled for these threads.  When constructing the signal frame, we leave
> the previous frame in place because we will return to it from the signal
> handler.  We also leave space for pushing eflags and the return address.
> UKL threads will only use the __KERNEL_DS value in the ss register and 0xC3
> in the cs register.

This is unclear.  Are you taking about returning from the kernel fault code *to* the signal handler or are you talking about returning *from* the user signal hander to the user code that was running when the signal happened?

In any case, I don't see what this has to do with iret or sysret.  Surely UKL can use a sigreturn() just like regular Linux.

The part where a UKL thread has permission to return to a CPL0 context should be a separate patch.

--Andy

Ali Raza Oct. 6, 2022, 9:20 p.m. UTC | #2

On 10/4/22 13:34, Andy Lutomirski wrote:
> 
> 
> On Mon, Oct 3, 2022, at 3:21 PM, Ali Raza wrote:
>> For a UKL thread, returning to a signal handler is not done with iret or
>> sysret.  This means we need to adjust the way the return stack frame is
>> handled for these threads.  When constructing the signal frame, we leave
>> the previous frame in place because we will return to it from the signal
>> handler.  We also leave space for pushing eflags and the return address.
>> UKL threads will only use the __KERNEL_DS value in the ss register and 0xC3
>> in the cs register.
> 
> This is unclear.  Are you taking about returning from the kernel fault code *to* the signal handler or are you talking about returning *from* the user signal hander to the user code that was running when the signal happened?
> 
> In any case, I don't see what this has to do with iret or sysret.  Surely UKL can use a sigreturn() just like regular Linux.
> 
> The part where a UKL thread has permission to return to a CPL0 context should be a separate patch.
> 
> --Andy

Yes, the commit message should have been clearer. 

The changes in __setup_rt_frame make sure that in case of a UKL thread,
the new frame should have the UKL specific regs->cs and regs->ds values,
and not have them overwritten with __USER_CS and __USER_DS. This helps
creating the correct iret frame in the interrupt return case where an
iret is used.

After the signal handler is invoked, user code calls sigreturn() as it
normally would. Once inside the rt_sigreturn() system call, UKL case is
handled a little different than normal. This is because UKL invokes
systems calls as function calls, so user stack gets a return address.
Also, UKL stores eflags on the user stack. This is used on return from
system calls in UKL, where we first switch to the user stack, then
restore flags through popfq. This restarts the interrupts so it is
important to have already switched to user stack from kernel stack. Once
flags are restored, we do a ret instead of iret. 

So, in rt_sigreturn() system call, we calculate the correct UKL regs->sp
by allowing space for the flags and return address on stack. Second, in
restore_sigcontext(), we again make sure that regs->cs and regs->ss are
only updated to user values for non UKL case.

Since, this patch involves both the signal handling and sigreturn case,
yes this can be broken into two patches.

Patch

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 9c7265b524c7..a95c12f6dac6 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -121,8 +121,10 @@  static bool restore_sigcontext(struct pt_regs *regs,
 #endif /* CONFIG_X86_64 */
 
 	/* Get CS/SS and force CPL3 */
-	regs->cs = sc.cs | 0x03;
-	regs->ss = sc.ss | 0x03;
+	if (!is_ukl_thread()) {
+		regs->cs = sc.cs | 0x03;
+		regs->ss = sc.ss | 0x03;
+	}
 
 	regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
 	/* disable syscall checks */
@@ -522,10 +524,15 @@  static int __setup_rt_frame(int sig, struct ksignal *ksig,
 	 * a trampoline.)  So we do our best: if the old SS was valid,
 	 * we keep it.  Otherwise we replace it.
 	 */
-	regs->cs = __USER_CS;
+	if (!is_ukl_thread()) {
+		regs->cs = __USER_CS;
 
-	if (unlikely(regs->ss != __USER_DS))
-		force_valid_ss(regs);
+		if (unlikely(regs->ss != __USER_DS))
+			force_valid_ss(regs);
+	} else {
+		regs->cs = 0xC3;
+		regs->ss = __KERNEL_DS;
+	}
 
 	return 0;
 
@@ -662,7 +669,10 @@  SYSCALL_DEFINE0(rt_sigreturn)
 	sigset_t set;
 	unsigned long uc_flags;
 
-	frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
+	if (is_ukl_thread())
+		frame = (struct rt_sigframe __user *)(regs->sp + sizeof(long));
+	else
+		frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
 	if (!access_ok(frame, sizeof(*frame)))
 		goto badframe;
 	if (__get_user(*(__u64 *)&set, (__u64 __user *)&frame->uc.uc_sigmask))