Oleg Nesterov <oleg@xxxxxxxxxx> 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
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
> 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;
> 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);
> }
