Received-SPF: pass (google.com: domain of linux-kernel+bounces-59689-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Christian Brauner <brauner@kernel.org>,  Andy Lutomirski
 <luto@amacapital.net>,  Tycho Andersen <tycho@tycho.pizza>,
  linux-api@vger.kernel.org,  linux-kernel@vger.kernel.org
References: <20240209130620.GA8039@redhat.com>
Date: Fri, 09 Feb 2024 10:22:23 -0600
In-Reply-To: <20240209130620.GA8039@redhat.com> (Oleg Nesterov's message of
	"Fri, 9 Feb 2024 14:06:20 +0100")
Message-ID: <87sf21zjy8.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v2 1/2] signal: add the "int si_code" arg to
 prepare_kill_siginfo()

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