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
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);
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);
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
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.
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
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
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.
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().