[0/1] *** Fix kill(-1,s) returning 0 on 0 kills ***

  Hi. I've never sent a kernel patch before but this one seemed trivial,
so I thought I'd give it a shot.

My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
to kill.

The code sample below demonstrates the problem, which gets fixed by the
patch:

    #define _GNU_SOURCE
    #include <assert.h>
    #include <errno.h>
    #include <signal.h>
    #include <stdio.h>
    #include <sys/wait.h>
    #include <unistd.h>
    #define VICTIM_UID 4200 //check these are safe to use on your system!
    #define UNUSED_UID 4300
    int main(){
        uid_t r,e,s;
        if(geteuid()) return 1; //requires root privileges

        //pipe to let the parent know when the child has changed ids
        int fds[2]; if(0>pipe(fds)) return 1;
        pid_t pid;
        if(0>(pid=fork())) return 1;
        else if(0==pid){
            setreuid(VICTIM_UID,VICTIM_UID);
            getresuid(&r,&e,&s); printf("child: %u %u %u\n", r,e,s);
            close(fds[0]); close(fds[1]); //let the parent continue
            for(;;) pause();
        }
        close(fds[1]);
        read(fds[0],&(char){0},1); //wait for uid change in the child

        #if 1
        setreuid(VICTIM_UID,(uid_t)-1); seteuid(VICTIM_UID);
        #else
        setresuid(UNUSED_UID,VICTIM_UID,0);
        #endif
        getresuid(&r,&e,&s); printf("parent: %u %u %u\n", r,e,s); //4200 4200 0

        int err = kill(-1,-111); (void)err; //test -EINVAL
        assert(err < 0 &&  errno == EINVAL);

        int rc = kill(-1,SIGTERM); //test 0
        if(rc>=0) wait(0);
        int rc2 = kill(-1,SIGTERM); //test -ESCHR
        printf("1st kill ok==%d; 2nd kill ESRCH==%d\n", rc==0, rc2<0&& errno==ESRCH);
    }

Thank you for considering the patch.

Best regards,
Petr S.


Petr Skocik (1):
  Fix kill(-1,s) returning 0 on 0 kills

 kernel/signal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
  

On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
> Hi. I've never sent a kernel patch before but this one seemed trivial,
> so I thought I'd give it a shot.
> 
> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
> to kill.

It looks like LTP already tests for this, and gets -ESRCH?
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c

Does it still pass with your change?
  

On 11/22/22 18:15, Kees Cook wrote:
> On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
>> Hi. I've never sent a kernel patch before but this one seemed trivial,
>> so I thought I'd give it a shot.
>>
>> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
>> to kill.
> It looks like LTP already tests for this, and gets -ESRCH?
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c
>
> Does it still pass with your change?
>
I went ahead and ran it and it does pass with the change.

But it should be obvious from the code alone too. It's only a few
(and fewer after the patch) simple lines of code.
The original:

         int retval = 0, count = 0;
         struct task_struct * p;

         for_each_process(p) {
             if (task_pid_vnr(p) > 1 &&
                     !same_thread_group(p, current)) {
                 int err = group_send_sig_info(sig, info, p,
                                   PIDTYPE_MAX);
                 ++count;
                 if (err != -EPERM)
                     retval = err;
             }
         }
         ret = count ? retval : -ESRCH;

counts kills made after the `task_pid_vnr(p) > 1 && 
!same_thread_group(p, current)` check.
Some, and possibly all, of those kills fail with -EPERM, but the the 
final line only sets -ESRCH
if the count is zero (i.e., the initial check fails). It should be 
counting only kill attempts that
have _not_ returned -EPERM (if all returned -EPERM, then no suitable 
target was found and
a -ESRCH is would be in order -- but it won't be set with the original 
code!).

So the change can be as minor as

     diff --git a/kernel/signal.c b/kernel/signal.c
     index d140672185a4..e42076e2332b 100644
     --- a/kernel/signal.c
     +++ b/kernel/signal.c
     @@ -1608,9 +1608,10 @@ static int kill_something_info(int sig, 
struct kernel_siginfo *info, pid_t pid)
                         !same_thread_group(p, current)) {
                     int err = group_send_sig_info(sig, info, p,
                                       PIDTYPE_MAX);
     -                ++count;
     -                if (err != -EPERM)
     +                if (err != -EPERM){
     +                    ++count;
                         retval = err;
     +                }
                 }
             }
             ret = count ? retval : -ESRCH;

But since the count variable isn't used other than for the zeroness 
check,  I simplified it further
into
     -        int retval = 0, count = 0;
             struct task_struct * p;

     +        ret = -ESRCH;
             for_each_process(p) {
                 if (task_pid_vnr(p) > 1 &&
                         !same_thread_group(p, current)) {
                     int err = group_send_sig_info(sig, info, p,
                                       PIDTYPE_MAX);
     -                ++count;
                     if (err != -EPERM)
     -                    retval = err;
     +                    ret = err; /*either all 0 or all -EINVAL*/
                 }
             }
     -        ret = count ? retval : -ESRCH;


adding a comment explaining the apparent implicit assumption of the 
original code that
the non-EPERM returns from group_send_sig_info in this context must be 
either all  -EINVAL
(bad signal number) or all 0, i.e., there can't be a signal allocation 
failure
(that would be susceptible to being overshadowed by a 0 returned from a 
later kill)
because none of  those kills in this context (kill not sigqueue) should 
require any memory allocation.

It's a tiny patch.

Cheers,
Petr Skocik

P.S.: Apologies if the code formatting is off. Sent this one with 
Thunderbird. Need to work on my
CLI mailsending skills.
  

Hi.

Is there anything else I can do to help get this (or some other 
equivalent change that results in kill(-1,s) returning -ESRCH when it 
has nothing to kill (like it does on the BSDs),
as opposed to the current return value of 0 in that case) incorporated 
into mainline Linux?

It would rather help some of the user software I'm developing, and the 
slightly new semantics are IMO definitely reasonable (BSDs have them).

Basically, the current code:
         int retval = 0, count = 0;
         struct task_struct * p;

         for_each_process(p) {
             if (task_pid_vnr(p) > 1 &&
                     !same_thread_group(p, current)) {
                 int err = group_send_sig_info(sig, info, p,
                                   PIDTYPE_MAX);
                 ++count;
                 if (err != -EPERM)
                     retval = err;
             }
         }
         ret = count ? retval : -ESRCH;

counts kill attempts at non-1, other-process pids  and sets hardcoded 
-ESRCH only if no such attempts are made, which will almost never happen

for a nonroot EUID, because there will typically be non-pid-1 processes 
unkillable by the nonroot EUID, but the code will still count those kill 
attempts, and thus not return the hardcoded -ESRCH even if ALL of those 
kill attemtpts return -EPERM, in which case -ESRCH would be in order 
too, because there were no processes that the current EUID had 
permission to kill (BDSs indeed return ESRCH in such a case).

(The kernel shouldn't need to concern itself with possible racy creation 
of new EUID-killable processes during the kill(-1,s) walk. Either the 
system can be known not to have running superuser code that could racily 
create such EUID-killable processes and then such a kill-returned -ESRCH 
would be useful, or it cannot be known not to have such running 
superuser code, in which case the -ESRCH is transient and should be 
droped by the user).

The current code also implicitly assumes either all non-EPERM kill 
attempts return -EINVAL (invalid signal) or they
all return 0 (success). This assumption should be valid because either 
the signal number is invalid and stays invalid, or it is valid and
the only possible error is -EPERM (this isn't sigqueue so the kill 
shouldn't ever fail with -ENOMEM). If the assumption were not valid,
then the current code could overshadow a previous failed attempt with a 
later succesful one, returning success even if there were some non-EPERM 
failures.

My change proposes:

         struct task_struct * p;

         ret = -ESRCH;
         for_each_process(p) {
             if (task_pid_vnr(p) > 1 &&
                     !same_thread_group(p, current)) {
                 int err = group_send_sig_info(sig, info, p,
                                   PIDTYPE_MAX);
                 if (err != -EPERM)
                     ret = err; /*either all 0 or all -EINVAL*/
             }
         }

i.e., start with -ESRCH (nothing to kill) and any non-EPERM kill 
attempts change it to the last return value
--either all 0 or all -EINVAL as per the implicit assumption of the 
original code.

It passes the tests put forth by Kees Cook.

More defensively, the implicit assumption of the original code could be 
made explicit:


         struct task_struct * p;
         int has_last_err = 0;

         ret = -ESRCH;
         for_each_process(p) {
             if (task_pid_vnr(p) > 1 &&
                     !same_thread_group(p, current)) {
                 int err = group_send_sig_info(sig, info, p,
                                   PIDTYPE_MAX);
                 if (err != -EPERM){
                     if (has_last_err)
                         BUG_ON(ret != err); /*either all 0 or all -EINVAL*/
                     has_last_err = 1;
                     ret = err;
                 }
             }
         }

or dropped;

         struct task_struct * p;
         int has_last_err = 0;

         ret = -ESRCH;
         for_each_process(p) {
             if (task_pid_vnr(p) > 1 &&
                     !same_thread_group(p, current)) {
                 int err = group_send_sig_info(sig, info, p,
                                   PIDTYPE_MAX);
                 if (err != -EPERM){
                     if (has_last_err){
                         if (err >= 0)
                             continue; /*don't mask previous failure 
with later success*/
                     }
                     has_last_err = 1;
                     ret = err;
                 }
             }
         }

Thanks again for consideration. Criticism welcome.

Regards,
Petr Skocik


On 11/22/22 18:15, Kees Cook wrote:
> On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
>> Hi. I've never sent a kernel patch before but this one seemed trivial,
>> so I thought I'd give it a shot.
>>
>> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
>> to kill.
> It looks like LTP already tests for this, and gets -ESRCH?
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c
>
> Does it still pass with your change?
>
  

Petr Skocik <pskocik@gmail.com> writes:

> Hi.
>
> Is there anything else I can do to help get this (or some other equivalent
> change that results in kill(-1,s) returning -ESRCH when it has nothing to kill
> (like it does on the BSDs),
> as opposed to the current return value of 0 in that case) incorporated into
> mainline Linux?

I think you are talking about a rare enough case that we can safely
change the error handling behavior  without real risk of trouble.

I think there is room for cleanup here.

I don't think we can change the set of processes signaled.  The linux
man page should be updated to note that we skip sending a signal
to ourselves in the event of -1.

Reading the code the error handling logic is dubious.

POSIX provides some guidance it says:

If pid is -1, sig shall be sent to all processes (excluding an
unspecified set of system processes) for which the process has
permission to send that signal.

[EINVAL]
    The value of the sig argument is an invalid or unsupported signal number.
[EPERM]
    The process does not have permission to send the signal to any receiving process.
[ESRCH]
    No process or process group can be found corresponding to that specified by pid. 

>                 if (err != -EPERM)
>                     ret = err; /*either all 0 or all -EINVAL*/

The comment in your proposed patch is wrong:
  -EAGAIN an be returned in the case of real time signals.
  -ENOMEM can be returned due to linux audit.
  -EINVAL can be returned, but arguably it should be caught
          before we even go into the loop.

Given that the comment is wrong I don't like what you have done with the
error handling logic.  It just adds confusion.

My question: What would a good and carefully implemented version
of kill(2) return?

Today for -pgrp we return 0 if any signal delivery succeeds and
the error from the last process in the signal group otherwise.

For -1 we return -EINVAL if the signal is invalid.
For -1 we return -ESRCH only if we are the init process and
there are no other processes in the system, aka never except
when we are the init process in a pid namespace.
For -1 otherwise we return the return value of the last
process signaled.

I would argue that what needs to happen for -1 is:
- Return 0 if the signal was sent to any process successfully.
- Return -EINVAL for invalid signals.
- When everything errors return some error value and not 0.

What error value should we return when everything errors?
Especially what error value should we return when everything
says -EPERM?

Should we follow BSD and return ESRCH?
Should we follow Posix and return EPERM?
Should we follow SYSV unix?

Looking at FreeBSD aka:
https://cgit.freebsd.org/src/tree/sys/kern/kern_sig.c?id=9e283cf9a2fe2b3aa6e4a47a012fd43b4e49ebec
kill(2) aka killpg1 only returns 0 or ESRCH when sending a signal
to multiple processes (after passing the EINVAL) check.

The man pages for AIX and Solaris suggest that -EPERM is returned when
things don't work.

So what should Linux do?

Historically Linux signaling is very SysV unix with a some compatibility
flags for BSD where it matters.  So I am not convinced that return
ESRCH in this case is the right answer.

Basing the logic off of __kill_pgrp_info I would do:

diff --git a/kernel/signal.c b/kernel/signal.c
index b5370fe5c198..369499ebb8e2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1602,7 +1602,8 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
 		ret = __kill_pgrp_info(sig, info,
 				pid ? find_vpid(-pid) : task_pgrp(current));
 	} else {
-		int retval = 0, count = 0;
+		bool found = false, success = false;
+		int retval = 0;
 		struct task_struct * p;
 
 		for_each_process(p) {
@@ -1610,12 +1611,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
 					!same_thread_group(p, current)) {
 				int err = group_send_sig_info(sig, info, p,
 							      PIDTYPE_MAX);
-				++count;
-				if (err != -EPERM)
-					retval = err;
+				found = true;
+				success |= !err;
+				retval = err;
 			}
 		}
-		ret = count ? retval : -ESRCH;
+		ret = success ? 0 : (found ? retval : -ESRCH);
 	}
 	read_unlock(&tasklist_lock);
 
I think that fits in better with Linux, and doesn't have any surprising
behavior.

> It would rather help some of the user software I'm developing, and the slightly
> new semantics are IMO definitely reasonable (BSDs have them).

Would my proposal above work for the software you are developing?

The behavior your patch was implementing was:
	ret = success ? 0 : ((retval == -EINVAL)? -EINVAL : -ESRCH);

Which gives less information.  So I am not thrilled by it.

Eric
  

On 8/10/23 18:16, Eric W. Biederman wrote:
> Petr Skocik <pskocik@gmail.com> writes:
>
>> Hi.
>>
>> Is there anything else I can do to help get this (or some other equivalent
>> change that results in kill(-1,s) returning -ESRCH when it has nothing to kill
>> (like it does on the BSDs),
>> as opposed to the current return value of 0 in that case) incorporated into
>> mainline Linux?
> I think you are talking about a rare enough case that we can safely
> change the error handling behavior  without real risk of trouble.
>
> I think there is room for cleanup here.
>
> I don't think we can change the set of processes signaled.  The linux
> man page should be updated to note that we skip sending a signal
> to ourselves in the event of -1.
>
> Reading the code the error handling logic is dubious.
>
> POSIX provides some guidance it says:
>
> If pid is -1, sig shall be sent to all processes (excluding an
> unspecified set of system processes) for which the process has
> permission to send that signal.
>
> [EINVAL]
>      The value of the sig argument is an invalid or unsupported signal number.
> [EPERM]
>      The process does not have permission to send the signal to any receiving process.
> [ESRCH]
>      No process or process group can be found corresponding to that specified by pid.
>
>>                  if (err != -EPERM)
>>                      ret = err; /*either all 0 or all -EINVAL*/
> The comment in your proposed patch is wrong:
>    -EAGAIN an be returned in the case of real time signals.
>    -ENOMEM can be returned due to linux audit.
>    -EINVAL can be returned, but arguably it should be caught
>            before we even go into the loop.
>
> Given that the comment is wrong I don't like what you have done with the
> error handling logic.  It just adds confusion.
>
> My question: What would a good and carefully implemented version
> of kill(2) return?
>
> Today for -pgrp we return 0 if any signal delivery succeeds and
> the error from the last process in the signal group otherwise.
>
> For -1 we return -EINVAL if the signal is invalid.
> For -1 we return -ESRCH only if we are the init process and
> there are no other processes in the system, aka never except
> when we are the init process in a pid namespace.
> For -1 otherwise we return the return value of the last
> process signaled.
>
> I would argue that what needs to happen for -1 is:
> - Return 0 if the signal was sent to any process successfully.
> - Return -EINVAL for invalid signals.
> - When everything errors return some error value and not 0.
>
> What error value should we return when everything errors?
> Especially what error value should we return when everything
> says -EPERM?
>
> Should we follow BSD and return ESRCH?
> Should we follow Posix and return EPERM?
> Should we follow SYSV unix?
>
> Looking at FreeBSD aka:
> https://cgit.freebsd.org/src/tree/sys/kern/kern_sig.c?id=9e283cf9a2fe2b3aa6e4a47a012fd43b4e49ebec
> kill(2) aka killpg1 only returns 0 or ESRCH when sending a signal
> to multiple processes (after passing the EINVAL) check.
>
> The man pages for AIX and Solaris suggest that -EPERM is returned when
> things don't work.
>
> So what should Linux do?
>
> Historically Linux signaling is very SysV unix with a some compatibility
> flags for BSD where it matters.  So I am not convinced that return
> ESRCH in this case is the right answer.
>
> Basing the logic off of __kill_pgrp_info I would do:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index b5370fe5c198..369499ebb8e2 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1602,7 +1602,8 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
>   		ret = __kill_pgrp_info(sig, info,
>   				pid ? find_vpid(-pid) : task_pgrp(current));
>   	} else {
> -		int retval = 0, count = 0;
> +		bool found = false, success = false;
> +		int retval = 0;
>   		struct task_struct * p;
>   
>   		for_each_process(p) {
> @@ -1610,12 +1611,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
>   					!same_thread_group(p, current)) {
>   				int err = group_send_sig_info(sig, info, p,
>   							      PIDTYPE_MAX);
> -				++count;
> -				if (err != -EPERM)
> -					retval = err;
> +				found = true;
> +				success |= !err;
> +				retval = err;
>   			}
>   		}
> -		ret = count ? retval : -ESRCH;
> +		ret = success ? 0 : (found ? retval : -ESRCH);
>   	}
>   	read_unlock(&tasklist_lock);
>   
> I think that fits in better with Linux, and doesn't have any surprising
> behavior.
>
>> It would rather help some of the user software I'm developing, and the slightly
>> new semantics are IMO definitely reasonable (BSDs have them).
> Would my proposal above work for the software you are developing?
>
> The behavior your patch was implementing was:
> 	ret = success ? 0 : ((retval == -EINVAL)? -EINVAL : -ESRCH);
>
> Which gives less information.  So I am not thrilled by it.
>
> Eric
>
>
>
Thanks for the detailed analysis, Eric W. Biederman.

All my software really cares about is that I get some indication that a 
kill(-1,s) run from a non-root pid no longer had anything left to kill, 
which on Linux is currently being masked by a return value of 0 whereas 
BDSs nicely provide an ESRCH. -EPERM would work too (and would still be 
more useful to me than the current behavior), but I will still object to 
it because I'm convinced you're misreading POSIX here and ESRCH, not 
EPERM, is the error that should be returned here.

You see, while the POSIX guidance for EPERM-returns from kill

     [EPERM] The process does not have permission to send the signal to 
any receiving process.

does indeed seem to suggest that EPERM might be right here, the issue is 
that the receiving processes that returned -EPERM in the loop were 
formally NOT the receiving processes of  kill(-1,s) at all

     If pid is -1, sig shall be sent to all processes (excluding an
     unspecified set of system processes) for which the process has
     permission to send that signal.

The -EPERM-returning internal kills (group_send_sig_info), according to 
the POSIX rules for kill(-1,s), *never* should have happened. It's 
harmless that they did happen as part of the implementation, given that 
those attempts aren't externally observable anyway, but this 
implementation detail should not leak out. Since all targets that for 
kill(-1,s) internally returned -EPERM formally shouldn't have been tried 
in the first place, then if all tried processes returned -EPERM, there 
was no process to try to kill and an -ESRCH is in order. No need to 
diverge from the BSDs here.

That is why the original code had a branch to disregard internal EPERM 
returns and why this branch should be preserved in any patches so that
kill(-1,s) should continue to NEVER return -EPERM. Returning it would 
contradict the spec (kill(-1,s) kills all it has permission to kill so 
it's nonsensical for it to report that it
lacks that permission).

As I said in previous messages, especially my latest one, I don't object 
to dropping the apparent implicit assumption of the current code that 
there can be no masking of a previous
non-EPERM error by a later success, or to making it explicit with some 
assert/BUG_ON statement. Please see the code examples for both of these 
other alternatives  in my previous message.

However, any other implementation (including the one you suggested) is 
welcome and thank you very much for your analysis and willingness to 
pick this.

Best regards,
Petr Skocik
  

Petr Skocik <pskocik@gmail.com> writes:

> Thanks for the detailed analysis, Eric W. Biederman.
>
> All my software really cares about is that I get some indication that a
> kill(-1,s) run from a non-root pid no longer had anything left to kill, 
> which on Linux is currently being masked by a return value of 0 whereas BDSs
> nicely provide an ESRCH. -EPERM would work too (and would still be more useful
> to me than the current behavior), but I will still object to it because I'm
> convinced you're misreading POSIX here and ESRCH, not EPERM, is the error that
> should be returned here.

Thank you for saying any error return is good enough for your
application.  It is definitely a bug that Linux reports success when no
signal has been delivered.

I dug into this a little bit more and found that Illumos and it's
ancestor OpenSolaris can return EPERM, even when sending to all
processes, by reading the Illumos source code.

Reading through the rational of kill it says that it is sometimes
desirable to hide the existence of one process from another so that the
existence of a process will not be an information leak.  To accommodate
that POSIX allows ESRCH instead of EPERM as an error code.

If you want you can read it for yourself here:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html


To sum up.

The function kill(2) should always report success when it has delivered
a signal and not otherwise.

The Linux version of kill(2) is buggy because it reports success when it
has not delivered a signal.

Different implementations of kill(2) do different things in this
situation and POSIX appears to allow the variation, so there is no
strong argument for any specific behavior (other than returning an
error) from a compatibility standpoint.

From my perspective making the implementation of sending a signal to all
processes (-1) handle errors the same as sending a signal to a process
group (-pgrp) seems like the most sensible way to fix this bug in Linux.

I can see an argument for hiding the existence of processes and
returning ESRCH but if/when we go down that road I would just ask that
we be consistent and update all of the signal sending functions at the
same time.

I will see about writing a commit message and posting a final patch in
just a little bit.

Eric
  

Thanks. I appreciate your patch and your researching of this.

I still think returning -EPERM for kill(-1,s) (unlike for kill(-pgrp,s), 
where it *can* make sense) is nonsensical because of how POSIX specifies 
kill(-1,sig) specifically ("sig shall be sent to all processes 
(excluding an unspecified set of system processes) for which the process 
has permission to send that signal"). But as I said, any error will do 
for me, so I am still grateful for your patch.

(The way I see it, the POSIX-mentioned possible hiding of processes via 
ESRCH is a completely different matter. In kill(-1,sig) specifically, 
targets that would return -EPERM are excluded/hidden by virtue of the 
definition of kill(-1,sig), which makes it different from other types of 
kills for which there's no generic need to hide EPERMs (only optional 
specific need, hence the paragraph in the POSIX spec on processes with a 
different security label)).

Best regards, Petr Skocik