linux-kernel - Re: Coverity: __do_sys_pidfd_send

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: <ZczLyDCN+zG6imTd@tycho.pizza>
Date: Wed, 14 Feb 2024 07:18:48 -0700
From: Tycho Andersen <tycho@...ho.pizza>
To: Oleg Nesterov <oleg@...hat.com>
Cc: coverity-bot <keescook@...omium.org>,
	Christian Brauner <brauner@...nel.org>,
	Nicholas Piggin <npiggin@...il.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Peng Zhang <zhangpeng.00@...edance.com>,
	Ard Biesheuvel <ardb@...nel.org>,
	Luis Chamberlain <mcgrof@...nel.org>,
	Heiko Carstens <hca@...ux.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Mateusz Guzik <mjguzik@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Tycho Andersen <tandersen@...flix.com>,
	Mike Christie <michael.christie@...cle.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	linux-kernel@...r.kernel.org,
	"Gustavo A. R. Silva" <gustavo@...eddedor.com>,
	linux-next@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

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