[PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

[PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

[Avi Weiss]

dequeue_synchronous_signal() uses the signal number and a positive
si_code to identify synchronous signals. An ordinarily queued
signal can also match this heuristic.

The initial pending-mask check only establishes that an unblocked
signal in SYNCHRONOUS_MASK is pending. The queue scan can select a
different blocked signal matching the heuristic and deliver it
despite its mask.

For example, a thread can block SIGSEGV and queue it to itself with
a positive si_code using rt_tgsigqueueinfo(). An unblocked SIGTRAP
then allows the scan to select the blocked SIGSEGV.

Check each candidate against the blocked mask. Genuine forced faults
are unblocked by force_sig_info_to_task() when necessary.

Fixes: 7146db3317c6 ("signal: Better detection of synchronous signals")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---

Reproduced by blocking SIGSEGV, queuing a positive-code SIGSEGV
with rt_tgsigqueueinfo(), then triggering an unblocked SIGTRAP.
Without the fix, SIGSEGV was delivered while still blocked.

Tested unpatched and patched kernels under QEMU on UP x86-64, with
test processes running as UID/GID 1000. The unpatched kernel passed
21/38 checks and the patched kernel passed 38/38.

These results are from the original patch base. The rebased v2 has
not been runtime-tested separately.

Changes in v2:
- Restore the original queue-scan comment following Eric's feedback.
- Clarify that ordinarily queued signals can match the
  synchronous-signal heuristic.
- Rebase onto current mainline.

 kernel/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index cb6ec7054eee..7e690f4fb5bc 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -689,7 +689,8 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
 	list_for_each_entry(q, &pending->list, list) {
 		/* Synchronous signals have a positive si_code */
 		if ((q->info.si_code > SI_USER) &&
-		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK)) {
+		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
+		    !sigismember(&tsk->blocked, q->info.si_signo)) {
 			sync = q;
 			goto next;
 		}

base-commit: 4403278e4ea2f654f8130a5c03eb8b4516a0f201
-- 
2.43.0

Re: [PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

[Oleg Nesterov]

On 10/22, Avi Weiss wrote:
>
> Changes in v2:
> - Restore the original queue-scan comment following Eric's feedback.
> - Clarify that ordinarily queued signals can match the
>   synchronous-signal heuristic.
> - Rebase onto current mainline.
>
>  kernel/signal.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index cb6ec7054eee..7e690f4fb5bc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -689,7 +689,8 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
>  	list_for_each_entry(q, &pending->list, list) {
>  		/* Synchronous signals have a positive si_code */
>  		if ((q->info.si_code > SI_USER) &&
> -		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK)) {
> +		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
> +		    !sigismember(&tsk->blocked, q->info.si_signo)) {

Still:

CC: stable@vger.kernel.org
Acked-by: Oleg Nesterov <oleg@redhat.com>


I was going to send another cleanup on top of this fix. Can't do this today,
but let me explain... It is a bit ugly to check si_signo twice. And afaics we
have another problem. sys_rt_sigqueueinfo() allows to send a SI_FROMKERNEL RT
signal to itself. In this case sigmask(q->info.si_signo) leads to UB on 32-bit.

I'll recheck, write the changelog, and send something like below tomorrow.
perhaps rt_sigmask() makes more sense than sigset_t...

Oleg.


From 6f7441ff08d7b88b96ac6787b40634e8c173003c Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Thu, 22 Oct 2026 13:43:01 +0200
Subject: [PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()

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

diff --git a/kernel/signal.c b/kernel/signal.c
index 7386b9552dc3..9cfd4d03cad4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -676,21 +676,24 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
 	struct task_struct *tsk = current;
 	struct sigpending *pending = &tsk->pending;
 	struct sigqueue *q, *sync = NULL;
+	unsigned long mask;
+	sigset_t xxx;
 
 	/*
 	 * Might a synchronous signal be in the queue?
 	 */
-	if (!((pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK))
+	mask = (pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK;
+	if (!mask)
 		return 0;
 
+	siginitset(&xxx, mask);
 	/*
 	 * Return the first synchronous signal in the queue.
 	 */
 	list_for_each_entry(q, &pending->list, list) {
 		/* Synchronous signals have a positive si_code */
-		if ((q->info.si_code > SI_USER) &&
-		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
-		    !sigismember(&tsk->blocked, q->info.si_signo)) {
+		if (SI_FROMKERNEL(&q->info) &&
+		    sigismember(&xxx, q->info.si_signo)) {
 			sync = q;
 			goto next;
 		}
-- 
2.55.0

Re: [PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

[Eric W. Biederman]

Avi Weiss <thnkslprpt@gmail.com> writes:

> dequeue_synchronous_signal() uses the signal number and a positive
> si_code to identify synchronous signals. An ordinarily queued
> signal can also match this heuristic.
>
> The initial pending-mask check only establishes that an unblocked
> signal in SYNCHRONOUS_MASK is pending. The queue scan can select a
> different blocked signal matching the heuristic and deliver it
> despite its mask.
>
> For example, a thread can block SIGSEGV and queue it to itself with
> a positive si_code using rt_tgsigqueueinfo(). An unblocked SIGTRAP
> then allows the scan to select the blocked SIGSEGV.
>
> Check each candidate against the blocked mask. Genuine forced faults
> are unblocked by force_sig_info_to_task() when necessary.
>
> Fixes: 7146db3317c6 ("signal: Better detection of synchronous signals")
> Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
> ---

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>
> Reproduced by blocking SIGSEGV, queuing a positive-code SIGSEGV
> with rt_tgsigqueueinfo(), then triggering an unblocked SIGTRAP.
> Without the fix, SIGSEGV was delivered while still blocked.
>
> Tested unpatched and patched kernels under QEMU on UP x86-64, with
> test processes running as UID/GID 1000. The unpatched kernel passed
> 21/38 checks and the patched kernel passed 38/38.
>
> These results are from the original patch base. The rebased v2 has
> not been runtime-tested separately.
>
> Changes in v2:
> - Restore the original queue-scan comment following Eric's feedback.
> - Clarify that ordinarily queued signals can match the
>   synchronous-signal heuristic.
> - Rebase onto current mainline.
>
>  kernel/signal.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index cb6ec7054eee..7e690f4fb5bc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -689,7 +689,8 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
>  	list_for_each_entry(q, &pending->list, list) {
>  		/* Synchronous signals have a positive si_code */
>  		if ((q->info.si_code > SI_USER) &&
> -		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK)) {
> +		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
> +		    !sigismember(&tsk->blocked, q->info.si_signo)) {
>  			sync = q;
>  			goto next;
>  		}
>
> base-commit: 4403278e4ea2f654f8130a5c03eb8b4516a0f201

Re: [PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> I was going to send another cleanup on top of this fix. Can't do this today,
> but let me explain... It is a bit ugly to check si_signo twice.

I can see where your proposed change is addressing that and it seems
reasonable.

> And afaics we have another problem. sys_rt_sigqueueinfo() allows to
> send a SI_FROMKERNEL RT signal to itself.

I don't see where your proposed code is addressing this.

From 10,000 feet I think it is deliberate that we allow sending all
possible pending signals to ourselves.  Some version of that is
necessary to restore a checkpoint of a process, if it has not
yet handled the signal at the point it is check-pointed.

You can look at CRIU if you need to see if this is behavior that
is being relied upon.

> In this case sigmask(q->info.si_signo) leads to UB on 32-bit.

I presume you are referring to a signal number that exceeds 32
being put into a 32bit mask.  That does seem worth fixing.

Eric


> I'll recheck, write the changelog, and send something like below tomorrow.
> perhaps rt_sigmask() makes more sense than sigset_t...
>
> Oleg.
>
>
> From 6f7441ff08d7b88b96ac6787b40634e8c173003c Mon Sep 17 00:00:00 2001
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Thu, 22 Oct 2026 13:43:01 +0200
> Subject: [PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7386b9552dc3..9cfd4d03cad4 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -676,21 +676,24 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
>  	struct task_struct *tsk = current;
>  	struct sigpending *pending = &tsk->pending;
>  	struct sigqueue *q, *sync = NULL;
> +	unsigned long mask;
> +	sigset_t xxx;
>  
>  	/*
>  	 * Might a synchronous signal be in the queue?
>  	 */
> -	if (!((pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK))
> +	mask = (pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK;
> +	if (!mask)
>  		return 0;
>  
> +	siginitset(&xxx, mask);
>  	/*
>  	 * Return the first synchronous signal in the queue.
>  	 */
>  	list_for_each_entry(q, &pending->list, list) {
>  		/* Synchronous signals have a positive si_code */
> -		if ((q->info.si_code > SI_USER) &&
> -		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
> -		    !sigismember(&tsk->blocked, q->info.si_signo)) {
> +		if (SI_FROMKERNEL(&q->info) &&
> +		    sigismember(&xxx, q->info.si_signo)) {
>  			sync = q;
>  			goto next;
>  		}