Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

From: Tycho Andersen <tycho@tycho.pizza>
To: Oleg Nesterov <oleg@redhat.com>
Cc: coverity-bot <keescook@chromium.org>,
	Christian Brauner <brauner@kernel.org>,
	Nicholas Piggin <npiggin@gmail.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Peng Zhang <zhangpeng.00@bytedance.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Heiko Carstens <hca@linux.ibm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Mateusz Guzik <mjguzik@gmail.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Tycho Andersen <tandersen@netflix.com>,
	Mike Christie <michael.christie@oracle.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	linux-kernel@vger.kernel.org,
	"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	linux-next@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT
Date: Wed, 14 Feb 2024 07:18:48 -0700	[thread overview]
Message-ID: <ZczLyDCN+zG6imTd@tycho.pizza> (raw)
In-Reply-To: <20240214090640.GB14017@redhat.com>

On Wed, Feb 14, 2024 at 10:06:41AM +0100, Oleg Nesterov wrote:
> On 02/14, Oleg Nesterov wrote:
> >
> > On 02/13, Tycho Andersen wrote:
> > >
> > > I think this is a false positive, we have:
> >
> > Agreed,
> >
> > > That said, a default case wouldn't hurt, and we should fix the first
> > > comment anyways, since now we have extensions.
> > >
> > > I'm happy to send a patch or maybe it's better for Christian to fix it
> > > in-tree.
> >
> > I leave this to you and Christian, whatever you prefer. But perhaps we
> > can simplify these checks? Something like below.
> 
> forgot about -EINVAL ...
> 
> Oleg.
> 
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3876,10 +3876,6 @@ static struct pid *pidfd_to_pid(const struct file *file)
>  	return tgid_pidfd_to_pid(file);
>  }
>  
> -#define PIDFD_SEND_SIGNAL_FLAGS                            \
> -	(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
> -	 PIDFD_SIGNAL_PROCESS_GROUP)
> -
>  /**
>   * sys_pidfd_send_signal - Signal a process through a pidfd
>   * @pidfd:  file descriptor of the process
> @@ -3903,13 +3899,23 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  	kernel_siginfo_t kinfo;
>  	enum pid_type type;
>  
> -	/* Enforce flags be set to 0 until we add an extension. */
> -	if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
> -		return -EINVAL;
> -
> -	/* Ensure that only a single signal scope determining flag is set. */
> -	if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
> +	switch (flags) {
> +	case 0:
> +		/* but see the PIDFD_THREAD check below */

Why not put that bit inline? But I guess the hweight and flags mask
are intended to be future proofness for flags that don't fit into this
switch. That said, your patch reads better than the way it is in the
tree and is what I was thinking.

Tycho