Coverity: __do_sys_pidfd_send_signal(): UNINIT

Coverity: __do_sys_pidfd_send_signal(): UNINIT

[coverity-bot]

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):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1583637 ("UNINIT")
Fixes: 3f643cd23510 ("pidfd: allow to override signal scope in pidfd_send_signal()")
Fixes: 81b9d8ac0640 ("pidfd: change pidfd_send_signal() to respect PIDFD_THREAD")

Thanks for your attention!

(Human note: looks like a default case is needed in the switch
statement.)

-- 
Coverity-bot

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Tycho Andersen]

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

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Oleg Nesterov]

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.

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,21 @@ 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)
-		return -EINVAL;
+	switch (flags) {
+	case 0:
+		/* but see the PIDFD_THREAD check below */
+		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;
+	}
 
 	f = fdget(pidfd);
 	if (!f.file)
@@ -3926,24 +3930,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 	if (!access_pidfd_pidns(pid))
 		goto err;
 
-	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:
+	if (!flags && (f.file->f_flags & PIDFD_THREAD))
 		type = PIDTYPE_PID;
-		break;
-	case PIDFD_SIGNAL_THREAD_GROUP:
-		type = PIDTYPE_TGID;
-		break;
-	case PIDFD_SIGNAL_PROCESS_GROUP:
-		type = PIDTYPE_PGID;
-		break;
-	}
 
 	if (info) {
 		ret = copy_siginfo_from_user_any(&kinfo, info);

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Oleg Nesterov]

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 */
+		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;
+	default:
 		return -EINVAL;
+	}
 
 	f = fdget(pidfd);
 	if (!f.file)
@@ -3926,24 +3932,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 	if (!access_pidfd_pidns(pid))
 		goto err;
 
-	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:
+	if (!flags && (f.file->f_flags & PIDFD_THREAD))
 		type = PIDTYPE_PID;
-		break;
-	case PIDFD_SIGNAL_THREAD_GROUP:
-		type = PIDTYPE_TGID;
-		break;
-	case PIDFD_SIGNAL_PROCESS_GROUP:
-		type = PIDTYPE_PGID;
-		break;
-	}
 
 	if (info) {
 		ret = copy_siginfo_from_user_any(&kinfo, info);

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Tycho Andersen]

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

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Oleg Nesterov]

Hi Tycho,

let me repeat just in case, I am fine either way, whatever you and
Christian prefer. In particular, I agree in advance if you decide
to not change the current code, it is correct even if it can fool
the tools.

That said,

On 02/14, Tycho Andersen wrote:
>
> On Wed, Feb 14, 2024 at 10:06:41AM +0100, Oleg Nesterov wrote:
> >
> > -	/* 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?

Not sure I understand what does "inline" mean... but let me reply
anyway.

We want to check the "flags" argument at the start, we do not want to
delay the "case 0:" check until we have f.file (so that we can check
f.file->f_flags).

but perhaps this is another case when I misunderstand you.

> But I guess the hweight and flags mask
> are intended to be future proofness for flags that don't fit into this
> switch.

Yes I see, but

> That said, your patch reads better than the way it is in the
> tree and is what I was thinking.

this was my point.

And if we add more flags, we will need to update the "switch" stmt anyway.

But again, I won't insist. This is cosmetic afer all.

Oleg.

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Tycho Andersen]

On Wed, Feb 14, 2024 at 06:55:55PM +0100, Oleg Nesterov wrote:
> Hi Tycho,
> 
> let me repeat just in case, I am fine either way, whatever you and
> Christian prefer. In particular, I agree in advance if you decide
> to not change the current code, it is correct even if it can fool
> the tools.
> 
> That said,
> 
> On 02/14, Tycho Andersen wrote:
> >
> > On Wed, Feb 14, 2024 at 10:06:41AM +0100, Oleg Nesterov wrote:
> > >
> > > -	/* 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?
> 
> Not sure I understand what does "inline" mean... but let me reply
> anyway.
> 
> We want to check the "flags" argument at the start, we do not want to
> delay the "case 0:" check until we have f.file (so that we can check
> f.file->f_flags).

Fair point. I was thinking delaying it would make it simpler, but then
you have to free the file and it's less fast in the EINVAL case. I
also don't have a strong opinion here.

Tycho

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Kees Cook]

On Tue, Feb 13, 2024 at 05:18:01PM -0700, Tycho Andersen wrote:
> 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;

Ah yeah, coverity can't see through the hweight32 test. Sorry for the
noise!

-- 
Kees Cook

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Oleg Nesterov]

On 02/14, Tycho Andersen wrote:
>
> On Wed, Feb 14, 2024 at 06:55:55PM +0100, Oleg Nesterov wrote:
> >
> > We want to check the "flags" argument at the start, we do not want to
> > delay the "case 0:" check until we have f.file (so that we can check
> > f.file->f_flags).
>
> Fair point. I was thinking delaying it would make it simpler, but then
> you have to free the file and it's less fast in the EINVAL case.

plus we do not want to return, say, -EBADF if the "flags" argument is wrong.

> I also don't have a strong opinion here.

Neither me.

Oleg.

Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT

[Christian Brauner]

On Wed, Feb 14, 2024 at 08:18:01PM +0100, Oleg Nesterov wrote:
> On 02/14, Tycho Andersen wrote:
> >
> > On Wed, Feb 14, 2024 at 06:55:55PM +0100, Oleg Nesterov wrote:
> > >
> > > We want to check the "flags" argument at the start, we do not want to
> > > delay the "case 0:" check until we have f.file (so that we can check
> > > f.file->f_flags).
> >
> > Fair point. I was thinking delaying it would make it simpler, but then
> > you have to free the file and it's less fast in the EINVAL case.
> 
> plus we do not want to return, say, -EBADF if the "flags" argument is wrong.
> 
> > I also don't have a strong opinion here.
> 
> Neither me.

Or you know, we just don't care about this. ;)
In any case since tis is a false positive it's not urgent in any way. If
either of you cares enough about this then please just send me patch that
reorders the checks to please that tool. The specific way doesn't matter
to me as well as long as we don't pointlessly fdget()/fdput().