[PATCH] rseq: x86: Fix rseq_cs get cleared when returning from signal handler

[Bui Quang Minh <minhquangbui99@gmail.com>]

rseq.rseq_cs is not expected to be cleared when we are still in critical
section. When using rseq with RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL, if the
signal is delivered, we don't restart to the abort ip but go to signal
handler then return and continue the critical section execution. While in
signal handler, if a preemption, migration or another signal is delivered,
we will fall into rseq_ip_fixup again. At that moment, the IP is in signal
handler not in critical section so the rseq.rseq_cs is cleared. Later, when
we return back to critical section from signal handlers, we get the
rseq.rseq_cs incorrectly cleared.

Another scenario is when a preemption or migration happens, then a signal
is delivered. When preparing to return to user mode, in
exit_to_user_mode_loop, we first handle the signal and set return IP to
signal handler. After that, rseq_handle_notify_resume is called to handle
the preemption/migration, sees the IP is the address of signal handler not
in critical section and clear rseq.rseq_cs.

To handle this case, a RSEQ_CS_FLAG_IN_SIGNAL_HANDLER is added. This flag
is set in rseq.flags (it is easier to read and update rseq.flags than
rseq.rseq_cs.flags) by the kernel when rseq does not restart on signal.
When this flag is set, we don't clear rseq.rseq_cs when the IP is outside
of critical section. The flag is unset in sigreturn path when the last
signal handler returns back to the critical section.

Reviewed-by: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 arch/x86/kernel/signal.c  |  7 ++++
 include/linux/sched.h     |  5 +++
 include/uapi/linux/rseq.h |  8 +++-
 kernel/rseq.c             | 80 ++++++++++++++++++++++++++++++++++++---
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 9c7265b524c7..7aadf69d6849 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -646,6 +646,10 @@ SYSCALL_DEFINE0(sigreturn)
 	 */
 	if (!restore_sigcontext(regs, &frame->sc, 0))
 		goto badframe;
+
+	if (rseq_sigreturn(regs))
+		goto badframe;
+
 	return regs->ax;
 
 badframe:
@@ -678,6 +682,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
+	if (rseq_sigreturn(regs))
+		goto badframe;
+
 	return regs->ax;
 
 badframe:
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c46f3a63b758..656b03232d0a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2340,6 +2340,8 @@ static inline void rseq_execve(struct task_struct *t)
 	t->rseq_event_mask = 0;
 }
 
+int rseq_sigreturn(struct pt_regs *regs);
+
 #else
 
 static inline void rseq_set_notify_resume(struct task_struct *t)
@@ -2365,6 +2367,9 @@ static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
 static inline void rseq_execve(struct task_struct *t)
 {
 }
+static inline int rseq_sigreturn(struct pt_regs *regs)
+{
+}
 
 #endif
 
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 77ee207623a9..f946af8e21e6 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -26,6 +26,7 @@ enum rseq_cs_flags_bit {
 	RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT	= 0,
 	RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT	= 1,
 	RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT	= 2,
+	RSEQ_CS_FLAG_IN_SIGNAL_HANDLER_BIT	= 3,
 };
 
 enum rseq_cs_flags {
@@ -35,6 +36,8 @@ enum rseq_cs_flags {
 		(1U << RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT),
 	RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE	=
 		(1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT),
+	RSEQ_CS_FLAG_IN_SIGNAL_HANDLER		=
+		(1U << RSEQ_CS_FLAG_IN_SIGNAL_HANDLER_BIT),
 };
 
 /*
@@ -115,7 +118,7 @@ struct rseq {
 	 * Restartable sequences flags field.
 	 *
 	 * This field should only be updated by the thread which
-	 * registered this data structure. Read by the kernel.
+	 * registered this data structure. Read and set by the kernel.
 	 * Mainly used for single-stepping through rseq critical sections
 	 * with debuggers.
 	 *
@@ -128,6 +131,9 @@ struct rseq {
 	 * - RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
 	 *     Inhibit instruction sequence block restart on migration for
 	 *     this thread.
+	 * - RSEQ_CS_FLAG_IN_SIGNAL_HANDLER
+	 *     Mark that this thread is in critical section but currently in
+	 *     signal handlers, so don't clear the rseq_cs field.
 	 */
 	__u32 flags;
 } __attribute__((aligned(4 * sizeof(__u64))));
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 97ac20b4f738..a4aca612724b 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -172,14 +172,16 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
 
 static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 {
-	u32 flags, event_mask;
+	u32 old_flags, flags, event_mask;
 	int ret;
 
 	/* Get thread flags. */
-	ret = get_user(flags, &t->rseq->flags);
+	ret = get_user(old_flags, &t->rseq->flags);
 	if (ret)
 		return ret;
 
+	flags = old_flags;
+
 	/* Take critical section flags into account. */
 	flags |= cs_flags;
 
@@ -203,6 +205,21 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	t->rseq_event_mask = 0;
 	preempt_enable();
 
+	/*
+	 * If we don't restart on signal event, set the
+	 * RSEQ_CS_FLAG_IN_SIGNAL_HANDLER flag so that later rseq_ip_fixup
+	 * doesn't clear rseq_cs pointer as the IP is outside of critical
+	 * section.
+	 */
+	if ((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
+	    (event_mask & RSEQ_EVENT_SIGNAL) &&
+	    !(old_flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER)) {
+		old_flags |= RSEQ_CS_FLAG_IN_SIGNAL_HANDLER;
+		ret = put_user(old_flags, &t->rseq->flags);
+		if (ret)
+			return ret;
+	}
+
 	return !!(event_mask & ~flags);
 }
 
@@ -248,10 +265,23 @@ static int rseq_ip_fixup(struct pt_regs *regs)
 	/*
 	 * Handle potentially not being within a critical section.
 	 * If not nested over a rseq critical section, restart is useless.
-	 * Clear the rseq_cs pointer and return.
+	 * In case the RSEQ_CS_FLAG_IN_SIGNAL_HANDLER is set, we are in
+	 * signal handlers and later return to critical section so don't
+	 * clear rseq_cs pointer.
+	 * Otherwise, clear the rseq_cs pointer and return.
 	 */
-	if (!in_rseq_cs(ip, &rseq_cs))
-		return clear_rseq_cs(t);
+	if (!in_rseq_cs(ip, &rseq_cs)) {
+		u32 flags;
+
+		ret = get_user(flags, &t->rseq->flags);
+		if (ret)
+			return ret;
+		if (flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER)
+			return 0;
+		else
+			return clear_rseq_cs(t);
+	}
+
 	ret = rseq_need_restart(t, rseq_cs.flags);
 	if (ret <= 0)
 		return ret;
@@ -322,6 +352,46 @@ void rseq_syscall(struct pt_regs *regs)
 
 #endif
 
+#ifdef CONFIG_RSEQ
+
+int rseq_sigreturn(struct pt_regs *regs)
+{
+	int ret;
+	struct rseq_cs rseq_cs;
+	struct task_struct *t = current;
+
+	if (t->rseq) {
+		u32 flags;
+
+		ret = get_user(flags, &t->rseq->flags);
+		if (ret)
+			return ret;
+
+		if (flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER) {
+			ret = rseq_get_rseq_cs(t, &rseq_cs);
+			if (ret)
+				return ret;
+
+			/*
+			 * If the returned IP is in critical section, it
+			 * means we handle all the possible nested signal,
+			 * so unset the RSEQ_CS_FLAG_IN_SIGNAL_HANDLER.
+			 */
+			if (in_rseq_cs(regs->ip, &rseq_cs)) {
+				flags &= ~RSEQ_CS_FLAG_IN_SIGNAL_HANDLER;
+
+				ret = put_user(flags, &t->rseq->flags);
+				if (ret)
+					return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+#endif
+
 /*
  * sys_rseq - setup restartable sequences for caller thread.
  */
-- 
2.25.1