Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Petr Skocik <pskocik@gmail.com>
Cc:     Kees Cook <keescook@chromium.org>, Oleg Nesterov <oleg@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Marco Elver <elver@google.com>, linux-kernel@vger.kernel.org
References: <20221122161240.137570-1-pskocik@gmail.com>
        <202211220913.AF86992@keescook>
        <d2d508b7-f267-0fe6-1b56-4292c95355a7@gmail.com>
        <878rai7u0l.fsf@email.froward.int.ebiederm.org>
        <336ae9be-c66c-d87f-61fe-b916e9f04ffc@gmail.com>
        <87pm3t2rvl.fsf@email.froward.int.ebiederm.org>
Date:   Fri, 11 Aug 2023 17:16:18 -0500
In-Reply-To: <87pm3t2rvl.fsf@email.froward.int.ebiederm.org> (Eric
        W. Biederman's message of "Fri, 11 Aug 2023 16:25:50 -0500")
Message-ID: <87jzu12pjh.fsf_-_@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: [PATCH] signal: Fix the error return of kill -1
Precedence: bulk


I dug through posix[1], the FreeBSD version of kill(2), and the Illumos
version of kill(2).  Common sense, the documentation and the other
implemnetations of kill(2) agree that an error should be returned if no
signal is delivered.

What is up in the air is which error code should be returned.  FreeBSD
uses ESRCH for all errors.  Illumos will return EPERM for some errors,
and ESRCH for others.  According to the rationale POSIX allows both.

The current Linux behavior of reporting success even when no signal
was delivered dates back to Linux 0.1 with the introduction of
returning ESRCH when there were no processes being added in Linux 1.0.

Since the current behavior is buggy and user-space cares[2][3] change
the behavior to match the behavior when Linux sends signals to process
groups.

Petr Skocik <pskocik@gmail.com> wrote:
> 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);
>     }

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html
[2] https://lkml.kernel.org/r/336ae9be-c66c-d87f-61fe-b916e9f04ffc@gmail.com
[3] https://lkml.kernel.org/r/20221122161240.137570-1-pskocik@gmail.com
Reported-by: Petr Skocik <pskocik@gmail.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index b5370fe5c198..731c6e3b351d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1582,8 +1582,9 @@ EXPORT_SYMBOL_GPL(kill_pid_usb_asyncio);
 /*
  * kill_something_info() interprets pid in interesting ways just like kill(2).
  *
- * POSIX specifies that kill(-1,sig) is unspecified, but what we have
- * is probably wrong.  Should make it like BSD or SYSV.
+ * POSIX allows the error codes EPERM and ESRCH when kill(-1,sig) does
+ * not deliver a signal to any process.  For consistency use the same
+ * logic in kill_something_info and __kill_pgrp_info.
  */
 
 static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
@@ -1602,7 +1603,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 +1612,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);
 
-- 
2.35.3