[PATCH] signal: add a helper for the si_code kernel impersonation check

[PATCH] signal: add a helper for the si_code kernel impersonation check

[Bradley Morgan]

The check that prevents userspace from sending siginfo with si_code
values reserved to the kernel is duplicated across do_rt_sigqueueinfo(),
do_rt_tgsigqueueinfo() and do_pidfd_send_signal(). Move the check
into a helper so the rule lives in one place.

Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/signal.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 9c2b32c4d755..b603d9b75aa3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3943,6 +3943,16 @@ static void prepare_kill_siginfo(int sig, struct kernel_siginfo *info,
 	info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
 }
 
+/*
+ * si_code values >= 0 and SI_TKILL are reserved to the kernel. Not even
+ * root can use them to pretend a signal was sent by the kernel or by
+ * kill()/tgkill(), except when signaling itself.
+ */
+static bool si_code_reserved_to_kernel(int si_code)
+{
+	return si_code >= 0 || si_code == SI_TKILL;
+}
+
 /**
  *  sys_kill - send a signal to a process
  *  @pid: the PID of the process
@@ -4038,7 +4048,7 @@ static int do_pidfd_send_signal(struct pid *pid, int sig, enum pid_type type,
 
 		/* Only allow sending arbitrary signals to yourself. */
 		if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
-		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
+		    si_code_reserved_to_kernel(kinfo.si_code))
 			return -EPERM;
 	} else {
 		prepare_kill_siginfo(sig, &kinfo, type);
@@ -4193,11 +4203,8 @@ SYSCALL_DEFINE2(tkill, pid_t, pid, int, sig)
 
 static int do_rt_sigqueueinfo(pid_t pid, int sig, kernel_siginfo_t *info)
 {
-	/* Not even root can pretend to send signals from the kernel.
-	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
-	 */
-	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
-	    (task_pid_vnr(current) != pid))
+	if (si_code_reserved_to_kernel(info->si_code) &&
+	    task_pid_vnr(current) != pid)
 		return -EPERM;
 
 	/* POSIX.1b doesn't mention process groups.  */
@@ -4240,11 +4247,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, kernel_siginfo_t
 	if (pid <= 0 || tgid <= 0)
 		return -EINVAL;
 
-	/* Not even root can pretend to send signals from the kernel.
-	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
-	 */
-	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
-	    (task_pid_vnr(current) != pid))
+	if (si_code_reserved_to_kernel(info->si_code) &&
+	    task_pid_vnr(current) != pid)
 		return -EPERM;
 
 	return do_send_specific(tgid, pid, sig, info);
-- 
2.53.0

Re: [PATCH] signal: add a helper for the si_code kernel impersonation check

[Oleg Nesterov]

On 07/06, Bradley Morgan wrote:
>
> +/*
> + * si_code values >= 0 and SI_TKILL are reserved to the kernel. Not even
> + * root can use them to pretend a signal was sent by the kernel or by
> + * kill()/tgkill(), except when signaling itself.
> + */
> +static bool si_code_reserved_to_kernel(int si_code)
> +{
> +	return si_code >= 0 || si_code == SI_TKILL;
> +}

...

>  static int do_rt_sigqueueinfo(pid_t pid, int sig, kernel_siginfo_t *info)
>  {
> -	/* Not even root can pretend to send signals from the kernel.
> -	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
> -	 */
> -	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
> -	    (task_pid_vnr(current) != pid))
> +	if (si_code_reserved_to_kernel(info->si_code) &&
> +	    task_pid_vnr(current) != pid)

Well, technically this (cosmetic) change is obviouly correct. And I do agree
it makes sense to factor out these checks and (more importantly) the comments.

But. IMO, the new comment looks a bit worse.

The old comment has "impersonate a kill()/tgkill(), which adds source info"
and this in fact explains that si_code == 0 or si_code == SI_TKILL come with
the real/valid .si_pid and .si_uid  and the reciever can trust them.

As for the naming... I would like to suggest a better name for the new helper
but I can't ;)

Oleg.

Re: [PATCH] signal: add a helper for the si_code kernel impersonation check

[Bradley Morgan]

On 5 August 2026 20:26:47 BST, Oleg Nesterov <oleg@redhat.com> wrote:
>On 07/06, Bradley Morgan wrote:
>>
>> +/*
>> + * si_code values >= 0 and SI_TKILL are reserved to the kernel. Not
>even
>> + * root can use them to pretend a signal was sent by the kernel or by
>> + * kill()/tgkill(), except when signaling itself.
>> + */
>> +static bool si_code_reserved_to_kernel(int si_code)
>> +{
>> +	return si_code >= 0 || si_code == SI_TKILL;
>> +}
>
>...
>
>>  static int do_rt_sigqueueinfo(pid_t pid, int sig, kernel_siginfo_t
>*info)
>>  {
>> -	/* Not even root can pretend to send signals from the kernel.
>> -	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
>> -	 */
>> -	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
>> -	    (task_pid_vnr(current) != pid))
>> +	if (si_code_reserved_to_kernel(info->si_code) &&
>> +	    task_pid_vnr(current) != pid)
>
>Well, technically this (cosmetic) change is obviouly correct. And I do
>agree
>it makes sense to factor out these checks and (more importantly) the
>comments.
>
>But. IMO, the new comment looks a bit worse.
>
>The old comment has "impersonate a kill()/tgkill(), which adds source
>info"
>and this in fact explains that si_code == 0 or si_code == SI_TKILL come
>with
>the real/valid .si_pid and .si_uid  and the reciever can trust them.
>

Hmm, what do you suggest?

>As for the naming... I would like to suggest a better name for the new
>helper
>but I can't ;)

Hmm, maybe AI could help with the bike shedding suggestion?

if not, I'll think of a new name and send V2 w the new name

>Oleg.
>
>

Thanks!

Re: [PATCH] signal: add a helper for the si_code kernel impersonation check

[Oleg Nesterov]

On 08/05, Bradley Morgan wrote:
>
> On 5 August 2026 20:26:47 BST, Oleg Nesterov <oleg@redhat.com> wrote:
> >On 07/06, Bradley Morgan wrote:
> >>
> >> +/*
> >> + * si_code values >= 0 and SI_TKILL are reserved to the kernel. Not
> >even
> >> + * root can use them to pretend a signal was sent by the kernel or by
> >> + * kill()/tgkill(), except when signaling itself.
> >> + */
> >> +static bool si_code_reserved_to_kernel(int si_code)
> >> +{
> >> +	return si_code >= 0 || si_code == SI_TKILL;
> >> +}
> >
> >...
> >
> >>  static int do_rt_sigqueueinfo(pid_t pid, int sig, kernel_siginfo_t
> >*info)
> >>  {
> >> -	/* Not even root can pretend to send signals from the kernel.
> >> -	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
> >> -	 */
> >> -	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
> >> -	    (task_pid_vnr(current) != pid))
> >> +	if (si_code_reserved_to_kernel(info->si_code) &&
> >> +	    task_pid_vnr(current) != pid)
> >
> >Well, technically this (cosmetic) change is obviouly correct. And I do
> >agree
> >it makes sense to factor out these checks and (more importantly) the
> >comments.
> >
> >But. IMO, the new comment looks a bit worse.
> >
> >The old comment has "impersonate a kill()/tgkill(), which adds source
> >info"
> >and this in fact explains that si_code == 0 or si_code == SI_TKILL come
> >with
> >the real/valid .si_pid and .si_uid  and the reciever can trust them.
> >
>
> Hmm, what do you suggest?

Hmm. it seems I wasn't clear...

If nothing else you can keep the old comment:

	* Not even root can pretend to send signals from the kernel.
	* Nor can they impersonate a kill()/tgkill(), which adds source info.

IMO, no need to add "si_code values >= 0 and SI_TKILL ..." at the start, this
just mirrors what the new (and trivial!) helper does.

May be you can improve it a bit, something like

	* Not even root can pretend to send SI_FROMKERNEL() signals.
	* Nor can they impersonate kill()/tgkill(), which have si_pid/uid

but this is subjective and minor.

I am fine either way. Just IMO the old comment(s) was more useful.

Oleg.