[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]

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;
>  		}

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

[Oleg Nesterov]

On 10/22, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > 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.

Hmm. It doesn't even try. And yes, I know it is deliberate, I know that
CRIU uses this feature.

> > 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.

Yes. That is all. I just tried to explain why this can happen.
sys_rt_sigqueueinfo() is the reason. The kernel doesn't sent
SI_FROMKERNEL signals with si_signo >= SIGRTMIN.

OK, let me write the changelog and simplify this change...

Oleg.

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

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 10/22, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > 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.
>
> Hmm. It doesn't even try. And yes, I know it is deliberate, I know that
> CRIU uses this feature.
>
>> > 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.
>
> Yes. That is all. I just tried to explain why this can happen.
> sys_rt_sigqueueinfo() is the reason. The kernel doesn't sent
> SI_FROMKERNEL signals with si_signo >= SIGRTMIN.

Oh I see.  I thought you were saying it was a bug that
sys_rt_sigqueue_info could send those signals, not that
it sending those signals would cause us problems.

Eric

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

[PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()

[Oleg Nesterov]

- Use SI_FROMKERNEL() instead of open-coded check

- Use rt_sigmask() instead of sigmask() to avoid UB on 32-bit if
  .si_signo > 32

- Pre-calculate the unblocked synchronous signal "mask" to not check
  .si_signo twice.

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

diff --git a/kernel/signal.c b/kernel/signal.c
index 7386b9552dc3..64259219bbdc 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -676,21 +676,17 @@ 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;
 
-	/*
-	 * Might a synchronous signal be in the queue?
-	 */
-	if (!((pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK))
+	/* Might a synchronous signal be in the queue? */
+	mask = (pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK;
+	if (!mask)
 		return 0;
 
-	/*
-	 * Return the first synchronous signal in the queue.
-	 */
+	/* 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) &&
+		    (rt_sigmask(q->info.si_signo) & mask)) {
 			sync = q;
 			goto next;
 		}
-- 
2.55.0

Re: [PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()

[Oleg Nesterov]

Christian,

This is on top of Avi's
[PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

If you are going to update your tree and take V2 + my cleanup, I will not
mind if you fold my patch into the fix from Avi.

And! I just noticed the sashiko's review of Avi's patch:
https://sashiko.dev/#/patchset/20261022061015.3737924-1-thnkslprpt%40gmail.com

It noticed the problem with UB too. But, unlike me, it noticed another problem
with the "still_pending" logic. The described scenario is not possible after my
change, but we have the same problem with posix timers.

This needs another change. I'll try to think a bit to avoid a "brute force" fix.

Oleg.

On 10/23, Oleg Nesterov wrote:
>
> - Use SI_FROMKERNEL() instead of open-coded check
> 
> - Use rt_sigmask() instead of sigmask() to avoid UB on 32-bit if
>   .si_signo > 32
> 
> - Pre-calculate the unblocked synchronous signal "mask" to not check
>   .si_signo twice.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7386b9552dc3..64259219bbdc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -676,21 +676,17 @@ 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;
>  
> -	/*
> -	 * Might a synchronous signal be in the queue?
> -	 */
> -	if (!((pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK))
> +	/* Might a synchronous signal be in the queue? */
> +	mask = (pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK;
> +	if (!mask)
>  		return 0;
>  
> -	/*
> -	 * Return the first synchronous signal in the queue.
> -	 */
> +	/* 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) &&
> +		    (rt_sigmask(q->info.si_signo) & mask)) {
>  			sync = q;
>  			goto next;
>  		}
> -- 
> 2.55.0
> 

Re: [PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> Christian,
>
> This is on top of Avi's
> [PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()
>
> If you are going to update your tree and take V2 + my cleanup, I will not
> mind if you fold my patch into the fix from Avi.
>
> And! I just noticed the sashiko's review of Avi's patch:
> https://sashiko.dev/#/patchset/20261022061015.3737924-1-thnkslprpt%40gmail.com
>
> It noticed the problem with UB too. But, unlike me, it noticed another problem
> with the "still_pending" logic. The described scenario is not possible after my
> change, but we have the same problem with posix timers.
>
> This needs another change. I'll try to think a bit to avoid a "brute
> force" fix.

One thing we can reasonably do is to disallow sending signals that
are "from the kernel" to ourselves if the signal number is a real time
signal.

Since the kernel won't generate any of those there is no need to restore
them, or test them.  So allowing them is unnecessary and leads to bugs
in the code.

I don't know how useful that would be but it is an option.

Eric