Re: [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill_siginfo()
From: Eric W. Biederman <hidden>
Date: 2024-02-09 17:00:00
Also in:
lkml
Possibly related (same subject, not in this thread)
- 2024-02-09 · Re: [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill_siginfo() · Christian Brauner <brauner@kernel.org>
- 2024-02-09 · Re: [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill_siginfo() · Christian Brauner <brauner@kernel.org>
- 2024-02-09 · [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill_siginfo() · Oleg Nesterov <oleg@redhat.com>
Oleg Nesterov [off-list ref] writes:
So that do_tkill() can use this helper too. This also simplifies the next patch. TODO: perhaps we can kill prepare_kill_siginfo() and change the callers to use SEND_SIG_NOINFO, but this needs some changes in __send_signal_locked() and TP_STORE_SIGINFO().
Could you can pass in the destination type instead of the si_code? Something like I have shown below? That allows the knowledge of the siginfo details to be isolated to prepare_kill_siginfo, making it easy to verify that a the union members match the union decode for the signal/si_code combination? It is all too easy to fill in siginfo improperly. Looking at siginfo_layout() SI_USER paired with any signal results in SIL_KILL whereas SI_TKILL paired with any signal results in SIL_RT. A superset of the fields of SIL_KILL. We use clear_siginfo() so si_sigval is set to 0 for SI_TKILL which seems correct. But we do allow userspace if it specifies SI_TKILL to provide si_sigval. So the current do_tkill code is very close to being wrong. Likewise you are filling in the details to match what the existing code is doing, so you are fine. Still it is a loaded footgun to allow passing in an arbitrary si_code. Eric
quoted hunk
Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)diff --git a/kernel/signal.c b/kernel/signal.c index c3fac06937e2..a8199fda0d61 100644 --- a/kernel/signal.c +++ b/kernel/signal.c@@ -3793,12 +3793,12 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait_time32, compat_sigset_t __user *, uthese, #endif #endif -static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info) +static void prepare_kill_siginfo(int sig, struct kernel_siginfo *info, int si_code) { clear_siginfo(info); info->si_signo = sig; info->si_errno = 0; - info->si_code = SI_USER; + info->si_code = si_code;
info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
quoted hunk
info->si_pid = task_tgid_vnr(current); info->si_uid = from_kuid_munged(current_user_ns(), current_uid()); }@@ -3812,7 +3812,7 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig) { struct kernel_siginfo info; - prepare_kill_siginfo(sig, &info); + prepare_kill_siginfo(sig, &info, SI_USER); return kill_something_info(sig, &info, pid); }@@ -3925,7 +3925,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) goto err; } else { - prepare_kill_siginfo(sig, &kinfo); + prepare_kill_siginfo(sig, &kinfo, SI_USER); } /* TODO: respect PIDFD_THREAD */@@ -3970,12 +3970,7 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig) { struct kernel_siginfo info; - clear_siginfo(&info); - info.si_signo = sig; - info.si_errno = 0; - info.si_code = SI_TKILL; - info.si_pid = task_tgid_vnr(current); - info.si_uid = from_kuid_munged(current_user_ns(), current_uid()); + prepare_kill_siginfo(sig, &info, SI_TKILL); return do_send_specific(tgid, pid, sig, &info); }