[PATCH v2 1/3] signal: change force_sig_info_to_task() to call __send_signal_locked()

[PATCH v2 1/3] signal: change force_sig_info_to_task() to call __send_signal_locked()

[Oleg Nesterov]

force_sig_info_to_task() calls send_signal_locked() which does two
things on top of __send_signal_locked():

1. The namespace translation of si_pid/si_uid. However, forced signals
   carry fault info (si_addr, si_call_addr, si_syscall), not pid/uid.
   The force_sig*() API should never be used to send signals with
   meaningful si_pid/si_uid, the forced signals are always "from kernel".

   There are few users of force_sig(SIGKILL), and in this case
   send_signal_locked() -> has_si_pid_and_uid() returns true.
   However, __send_signal_locked() simply ignores kernel_siginfo if
   sig == SIGKILL.

   (and in fact force_sig(SIGKILL) makes little sense, they should
    use send_sig(SIGKILL, p, 1) instead)

2. The "force" computation. However, for the forced signals, the
   unconditional force == true works just fine.

   If the target is ptraced, the "force" arg has no effect unless
   sig == SIGKILL.

   Otherwise, this check in sig_task_ignored()

	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) &&
	    handler == SIG_DFL && !(force && sig_kernel_only(sig)))
		return true;

   has no effect, force_sig_info_to_task() clears SIGNAL_UNKILLABLE
   if handler == SIG_DFL.

   The only behavioral difference is another check in sig_task_ignored:

	if (unlikely((t->flags & PF_KTHREAD) &&
		     (handler == SIG_KTHREAD_KERNEL) && !force))

   So with this patch a kthread that called allow_kernel_signal()
   for a fault signal would now receive the forced signal instead
   of silently ignoring it.

   And this is arguably more correct, even if I don't think that
   the force_sig*() API should be used in this case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 9c2b32c4d755..68af503ed43c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1315,7 +1315,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
 	if (action->sa.sa_handler == SIG_DFL &&
 	    (!t->ptrace || (handler == HANDLER_EXIT)))
 		t->signal->flags &= ~SIGNAL_UNKILLABLE;
-	ret = send_signal_locked(sig, info, t, PIDTYPE_PID);
+	ret = __send_signal_locked(sig, info, t, PIDTYPE_PID, true);
 	/* This can happen if the signal was already pending and blocked */
 	if (!task_sigpending(t))
 		signal_wake_up(t, 0);
-- 
2.52.0

[PATCH v2 2/3] signal: turn the "bool force" arg of __send_signal_locked() into "int flags"

[Oleg Nesterov]

No functional change. Preparation for the next patch which will add another
flag to fix the SA_IMMUTABLE signal evasion.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 68af503ed43c..9c607a598ba1 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1037,8 +1037,10 @@ static inline bool legacy_queue(struct sigpending *signals, int sig)
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+#define SEND_SIGNAL_FORCE	(1 << 0)
+
 static int __send_signal_locked(int sig, struct kernel_siginfo *info,
-				struct task_struct *t, enum pid_type type, bool force)
+				struct task_struct *t, enum pid_type type, int flags)
 {
 	struct sigpending *pending;
 	struct sigqueue *q;
@@ -1048,7 +1050,7 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 	lockdep_assert_held(&t->sighand->siglock);
 
 	result = TRACE_SIGNAL_IGNORED;
-	if (!prepare_signal(sig, t, force))
+	if (!prepare_signal(sig, t, flags & SEND_SIGNAL_FORCE))
 		goto ret;
 
 	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
@@ -1211,7 +1213,8 @@ int send_signal_locked(int sig, struct kernel_siginfo *info,
 			force = true;
 		}
 	}
-	return __send_signal_locked(sig, info, t, type, force);
+	return __send_signal_locked(sig, info, t, type,
+				    force ? SEND_SIGNAL_FORCE : 0);
 }
 
 static void print_fatal_signal(int signr)
@@ -1295,6 +1298,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
 	unsigned long int flags;
 	int ret, blocked, ignored;
 	struct k_sigaction *action;
+	int send_flags = SEND_SIGNAL_FORCE;
 	int sig = info->si_signo;
 
 	spin_lock_irqsave(&t->sighand->siglock, flags);
@@ -1315,7 +1319,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
 	if (action->sa.sa_handler == SIG_DFL &&
 	    (!t->ptrace || (handler == HANDLER_EXIT)))
 		t->signal->flags &= ~SIGNAL_UNKILLABLE;
-	ret = __send_signal_locked(sig, info, t, PIDTYPE_PID, true);
+	ret = __send_signal_locked(sig, info, t, PIDTYPE_PID, send_flags);
 	/* This can happen if the signal was already pending and blocked */
 	if (!task_sigpending(t))
 		signal_wake_up(t, 0);
@@ -1550,7 +1554,7 @@ int kill_pid_usb_asyncio(int sig, int errno, sigval_t addr,
 
 	if (sig) {
 		if (lock_task_sighand(p, &flags)) {
-			ret = __send_signal_locked(sig, &info, p, PIDTYPE_TGID, false);
+			ret = __send_signal_locked(sig, &info, p, PIDTYPE_TGID, 0);
 			unlock_task_sighand(p, &flags);
 		} else
 			ret = -ESRCH;
@@ -2259,7 +2263,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
 	 * parent's namespaces.
 	 */
 	if (sig)
-		__send_signal_locked(sig, &info, tsk->parent, PIDTYPE_TGID, false);
+		__send_signal_locked(sig, &info, tsk->parent, PIDTYPE_TGID, 0);
 	__wake_up_parent(tsk, tsk->parent);
 	spin_unlock_irqrestore(&psig->siglock, flags);
 
-- 
2.52.0

[PATCH v2 3/3] signal: fix evasion of SA_IMMUTABLE signals

[Oleg Nesterov]

force_sig_info_to_task(HANDLER_EXIT) sets SA_IMMUTABLE to ensure a forced
fatal signal cannot be ignored or caught by userspace; it must always
terminate the target. However, if get_signal() dequeues another synchronous
signal first, and that signal has a handler and its sa_mask includes the
fatal SA_IMMUTABLE signal, the task can return to userspace and survive.

So dequeue_synchronous_signal() must always dequeue an SA_IMMUTABLE signal
first. But it relies on the SI_FROMKERNEL() check and picks the first one
it sees in pending->list, and thus we have the following problems:

- If the same signal was already pending and blocked, the new siginfo
  with .si_code > 0 will be lost.

  Change __send_signal_locked() to bypass the legacy_queue() check in
  this case.

- If force_sig_info_to_task() races with another synchronous/SI_FROMKERNEL
  signal, that signal can be picked first.

  Change __send_signal_locked() to add an SA_IMMUTABLE signal at the start
  of pending->list.

- SA_IMMUTABLE implies override_rlimit == true, but GFP_ATOMIC can fail
  anyway.

  Change __send_signal_locked() to escalate to SIGKILL in this (very
  unlikely) case.

  Not perfect and perhaps deserves WARN() or pr_warn_ratelimited(), but
  better than nothing.

However, unlike get_signal(), __send_signal_locked() can not rely on the
k_sigaction.sa.sa_flags & SA_IMMUTABLE check; another signal with the same
.si_signo can come before dequeue_synchronous_signal() dequeues the signal
sent by force(HANDLER_EXIT). Say, send_sig_perf() from task_work_run(),
and this signal is SI_FROMKERNEL() too.

Use the new SEND_SIGNAL_IMMUTABLE flag to pass the "immutable" state from
force_sig_info_to_task() to __send_signal_locked().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 9c607a598ba1..077effd21582 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1038,10 +1038,12 @@ static inline bool legacy_queue(struct sigpending *signals, int sig)
 }
 
 #define SEND_SIGNAL_FORCE	(1 << 0)
+#define SEND_SIGNAL_IMMUTABLE	(1 << 1)
 
 static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 				struct task_struct *t, enum pid_type type, int flags)
 {
+	bool immutable = flags & SEND_SIGNAL_IMMUTABLE;
 	struct sigpending *pending;
 	struct sigqueue *q;
 	int override_rlimit;
@@ -1055,12 +1057,12 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 
 	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
 	/*
-	 * Short-circuit ignored signals and support queuing
-	 * exactly one non-rt signal, so that we can get more
-	 * detailed information about the cause of the signal.
+	 * Queue exactly one non-rt signal so that we can get more
+	 * detailed information about the cause. But we must never
+	 * lose the siginfo for an SA_IMMUTABLE signal.
 	 */
 	result = TRACE_SIGNAL_ALREADY_PENDING;
-	if (legacy_queue(pending, sig))
+	if (legacy_queue(pending, sig) && !immutable)
 		goto ret;
 
 	result = TRACE_SIGNAL_DELIVERED;
@@ -1087,7 +1089,12 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 	q = sigqueue_alloc(sig, t, GFP_ATOMIC, override_rlimit);
 
 	if (q) {
-		list_add_tail(&q->list, &pending->list);
+		/* Ensure dequeue_synchronous_signal() sees SA_IMMUTABLE first */
+		if (immutable)
+			list_add(&q->list, &pending->list);
+		else
+			list_add_tail(&q->list, &pending->list);
+
 		switch ((unsigned long) info) {
 		case (unsigned long) SEND_SIG_NOINFO:
 			clear_siginfo(&q->info);
@@ -1130,6 +1137,9 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 		 * send the signal, but the *info bits are lost.
 		 */
 		result = TRACE_SIGNAL_LOSE_INFO;
+		/* The task must not escape SA_IMMUTABLE; escalate to SIGKILL */
+		if (immutable)
+			sig = SIGKILL;
 	}
 
 out_set:
@@ -1307,8 +1317,10 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
 	blocked = sigismember(&t->blocked, sig);
 	if (blocked || ignored || (handler != HANDLER_CURRENT)) {
 		action->sa.sa_handler = SIG_DFL;
-		if (handler == HANDLER_EXIT)
+		if (handler == HANDLER_EXIT) {
 			action->sa.sa_flags |= SA_IMMUTABLE;
+			send_flags |= SEND_SIGNAL_IMMUTABLE;
+		}
 		if (blocked)
 			sigdelset(&t->blocked, sig);
 	}
-- 
2.52.0