Re: [RFC PATCH v2 29/41] signal: report the sender's UUID or GUID uid in si_uid_r

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] signal: broken userns UID mapping in send_signal_locked() and callers
- [Medium] bpf: missing siginfo_set_kernel_uid() call in bpf_send_signal_common()
--

commit 3fc8400fa91445ed3a591553195996d33af32cb3
Author: Jan Koester <jan.koester@tuxist.de>

signal: report the sender's UUID or GUID uid in si_uid_r

This commit adds si_uid_r (__kernel_uid_r_t) at offset 96 in siginfo_t and
compat_siginfo_t, along with ssi_uid_r in signalfd_siginfo, to report the
sender's UUID or GUID UID for signals and waitid(). It introduces
siginfo_set_uid() and siginfo_set_kernel_uid() to populate and translate
si_uid and si_uid_r across signal generation, delivery, and wait paths.

> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index f19816832f055..928e67e283b60 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -6,6 +6,7 @@
>  #include <linux/list.h>
>  #include <linux/signal_types.h>
>  #include <linux/string.h>
> +#include <linux/uidgid.h>
>  
>  struct task_struct;
>  
> @@ -23,6 +24,27 @@ static inline void clear_siginfo(kernel_siginfo_t *info)
>  	memset(info, 0, sizeof(*info));
>  }
>  
> +struct user_namespace;
> +
> +/*
> + * Sets si_uid and si_uid_r to @uid as seen from @ns (the receiver's user
> + * namespace), the overflow id if it has no mapping there.
> + */
> +static inline void siginfo_set_uid(kernel_siginfo_t *info,
> +				   struct user_namespace *ns, kuid_t uid)
> +{
> +	info->si_uid = from_kuid_munged(ns, uid);
> +	from_kuid_r_munged(ns, uid, &info->si_uid_r);
> +}
> +
> +/* si_uid and si_uid_r of a signal from the kernel: 0 */
> +static inline void siginfo_set_kernel_uid(kernel_siginfo_t *info)
> +{
> +	info->si_uid = 0;
> +	memset(&info->si_uid_r, 0, sizeof(info->si_uid_r));
> +	info->si_uid_r.type = UID_R_LEGACY;
> +}

[Severity: Medium]
Should bpf_send_signal_common() in kernel/trace/bpf_trace.c also call
siginfo_set_kernel_uid(&info) when sending a signal to a specific task?

When a BPF program invokes bpf_send_signal_task() with a non-NULL task,
bpf_send_signal_common() takes the else branch and initializes info manually:

kernel/trace/bpf_trace.c:bpf_send_signal_common() {
	if (!task) {
		task = current;
		siginfo = SEND_SIG_PRIV;
	} else {
		clear_siginfo(&info);
		info.si_signo = sig;
		info.si_errno = 0;
		info.si_code = SI_KERNEL;
		info.si_pid = 0;
		info.si_uid = 0;
		info.si_value.sival_ptr = (void __user __force *)(unsigned long)value;
		siginfo = &info;
	}
}

Because info.si_uid = 0 is set directly after clear_siginfo(&info),
info.si_uid_r remains UID_R_NONE instead of UID_R_LEGACY with uid 0, whereas
calling bpf_send_signal() or bpf_send_signal_thread() (where task is NULL)
uses SEND_SIG_PRIV and sets info.si_uid_r via siginfo_set_kernel_uid().

[ ... ]

> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index d1dd36a651b0d..f0a319ed97515 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -802,8 +802,8 @@ static void __do_notify(struct mqueue_inode_info *info)
>  			/* map current pid/uid into info->owner's namespaces */
>  			sig_i.si_pid = task_tgid_nr_ns(current,
>  						ns_of_pid(info->notify_owner));
> -			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
> -						current_uid());
> +			siginfo_set_uid(&sig_i, info->notify_user_ns,
> +					current_uid());
>  			/*
>  			 * We can't use kill_pid_info(), this signal should
>  			 * bypass check_kill_permission(). It is from kernel

[ ... ]

> diff --git a/kernel/signal.c b/kernel/signal.c
> index d31ebcb6ed4db..af0793d04b640 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c

[ ... ]

> @@ -1229,6 +1228,13 @@ int send_signal_locked(int sig, struct kernel_siginfo *info,
>  			info = &rewritten;
>  			uid = make_kuid(current_user_ns(), info->si_uid);
>  			rewritten.si_uid = from_kuid_munged(t_user_ns, uid);
> +			if (rewritten.si_uid_r.type != UID_R_NONE) {
> +				kuid_t uid_r = make_kuid_r(current_user_ns(),
> +							   &rewritten.si_uid_r);
> +
> +				from_kuid_r_munged(t_user_ns, uid_r,
> +						   &rewritten.si_uid_r);
> +			}
>  		}
>  		rcu_read_unlock();
>  #endif

[Severity: High]
Can this cross-user-namespace translation in send_signal_locked() corrupt
both si_uid and si_uid_r in several signal delivery paths?

First, in send_signal_locked(), rewritten.si_uid is still derived from the
32-bit info->si_uid via make_kuid(current_user_ns(), info->si_uid) rather
than from uid_r. When a process with a UUID or GUID UID in init_user_ns
signals a task in a child user namespace t_user_ns (via
prepare_kill_siginfo() in tkill(), tgkill(), or pidfd_send_signal() with
info == NULL, or via rt_sigqueueinfo()), info->si_uid has already been munged
to overflowuid (65534). make_kuid(&init_user_ns, 65534) then fabricates the
legacy host UID KUIDT_INIT(65534). If t_user_ns maps legacy host UID 65534 to
a valid container UID, rewritten.si_uid is set to that container UID instead
of overflowuid, while rewritten.si_uid_r is munged to overflowuid.

Second, prepare_kill_siginfo() in kernel/signal.c pre-maps current_uid() into
current_user_ns() before send_signal_locked() knows the target's user
namespace:

kernel/signal.c:prepare_kill_siginfo() {
	clear_siginfo(info);
	info->si_signo = sig;
	info->si_errno = 0;
	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
	info->si_pid = task_tgid_vnr(current);
	siginfo_set_uid(info, current_user_ns(), current_uid());
}

If a task with a UUID or GUID UID enters a child user namespace via
unshare(CLONE_NEWUSER) or setns() and calls tkill(), tgkill(), or
pidfd_send_signal() (with info == NULL) targeting a task in init_user_ns,
siginfo_set_uid() in prepare_kill_siginfo() immediately munges info->si_uid_r
to UID_R_LEGACY / overflowuid because current_user_ns() != &init_user_ns.
When send_signal_locked() subsequently translates from current_user_ns() to
t_user_ns (&init_user_ns), the original UUID or GUID has already been lost.

Third, both do_notify_parent_cldstop() in kernel/signal.c and __do_notify()
in ipc/mqueue.c already translate the sender's kuid_t into the receiver's
user namespace via siginfo_set_uid(), but then pass the resulting siginfo to
send_signal_locked():

kernel/signal.c:do_notify_parent_cldstop()
  -> siginfo_set_uid(&info, task_cred_xxx(parent, user_ns), task_uid(tsk))
  -> send_signal_locked(SIGCHLD, &info, parent, PIDTYPE_TGID)

ipc/mqueue.c:__do_notify()
  -> siginfo_set_uid(&sig_i, info->notify_user_ns, current_uid())
  -> do_send_sig_info(info->notify.sigev_signo, &sig_i, task, PIDTYPE_TGID)
    -> send_signal_locked(sig, info, p, type)

When current_user_ns() != t_user_ns, send_signal_locked() translates si_uid
and si_uid_r a second time assuming they were formatted in current_user_ns().
For example, if a child or mqueue sender with a UUID or GUID UID is in a
child user namespace and the parent or notify_owner is in init_user_ns,
siginfo_set_uid() writes the UUID or GUID into si_uid_r, and
send_signal_locked() then calls make_kuid_r(current_user_ns(), ...), which
fails in kid_r_to_128() and returns INVALID_UID, overwriting the valid UUID
or GUID with overflowuid.

[ ... ]

完美！ Let's double check everything before finalizing. Everything adheres strictly to the guidelines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=29