Received-SPF: pass (google.com: domain of linux-kernel+bounces-64604-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i21f147d5:Fastmail
Date: Tue, 13 Feb 2024 17:18:01 -0700
From: Tycho Andersen <tycho@tycho.pizza>
To: coverity-bot <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>, 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
Message-ID: <ZcwGua3a9Z8nJXVq@tycho.pizza>
References: <202402131559.B76A34B@keescook>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202402131559.B76A34B@keescook>

On Tue, Feb 13, 2024 at 03:59:37PM -0800, coverity-bot wrote:
> Hello!
> 
> This is an experimental semi-automated report about issues detected by
> Coverity from a scan of next-20240213 as part of the linux-next scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
> 
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by commits:
> 
>   Sat Feb 10 22:37:25 2024 +0100
>     3f643cd23510 ("pidfd: allow to override signal scope in pidfd_send_signal()")
>   Sat Feb 10 22:37:23 2024 +0100
>     81b9d8ac0640 ("pidfd: change pidfd_send_signal() to respect PIDFD_THREAD")
> 
> Coverity reported the following:
> 
> *** CID 1583637:    (UNINIT)
> kernel/signal.c:3963 in __do_sys_pidfd_send_signal()
> 3957     		/* Only allow sending arbitrary signals to yourself. */
> 3958     		ret = -EPERM;
> 3959     		if ((task_pid(current) != pid) &&
> 3960     		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
> 3961     			goto err;
> 3962     	} else {
> vvv     CID 1583637:    (UNINIT)
> vvv     Using uninitialized value "type" when calling "prepare_kill_siginfo".
> 3963     		prepare_kill_siginfo(sig, &kinfo, type);
> 3964     	}
> 3965
> 3966     	if (type == PIDTYPE_PGID)
> 3967     		ret = kill_pgrp_info(sig, &kinfo, pid);
> 3968     	else
> kernel/signal.c:3966 in __do_sys_pidfd_send_signal()
> 3960     		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
> 3961     			goto err;
> 3962     	} else {
> 3963     		prepare_kill_siginfo(sig, &kinfo, type);
> 3964     	}
> 3965
> vvv     CID 1583637:    (UNINIT)
> vvv     Using uninitialized value "type".
> 3966     	if (type == PIDTYPE_PGID)
> 3967     		ret = kill_pgrp_info(sig, &kinfo, pid);
> 3968     	else
> 3969     		ret = kill_pid_info_type(sig, &kinfo, pid, type);
> 3970     err:
> 3971     	fdput(f);
> 
> If this is a false positive, please let us know so we can mark it as
> such, or teach the Coverity rules to be smarter. If not, please make
> sure fixes get into linux-next. :) For patches fixing this, please
> include these lines (but double-check the "Fixes" first):

I think this is a false positive, we have:

        /* 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)
                return -EINVAL;

which should enforce that at most one bit is set, and there's a case
statement for each of these bits in the later switch,

        switch (flags) {
        case 0:
                /* Infer scope from the type of pidfd. */
                if (f.file->f_flags & PIDFD_THREAD)
                        type = PIDTYPE_PID;
                else
                        type = PIDTYPE_TGID;
                break;
        case PIDFD_SIGNAL_THREAD:
                type = PIDTYPE_PID;
                break;
        case PIDFD_SIGNAL_THREAD_GROUP:
                type = PIDTYPE_TGID;
                break;
        case PIDFD_SIGNAL_PROCESS_GROUP:
                type = PIDTYPE_PGID;
                break;
        }

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.

Tycho