linux-kernel - Re: [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sf21zjy8.fsf@email.froward.int.ebiederm.org>
Date: Fri, 09 Feb 2024 10:22:23 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Christian Brauner <brauner@...nel.org>,  Andy Lutomirski
 <luto@...capital.net>,  Tycho Andersen <tycho@...ho.pizza>,
  linux-api@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] signal: add the "int si_code" arg to
 prepare_kill_siginfo()

Oleg Nesterov <oleg@...hat.com> 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@...hat.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;
>  	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);
>  }