[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4e1dcf10-dba0-4cd4-cfa7-db6b8e06eeff@gmail.com>
Date: Wed, 23 Nov 2022 00:01:39 +0100
From: Petr Skocik <pskocik@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>,
Oleg Nesterov <oleg@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Marco Elver <elver@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] *** Fix kill(-1,s) returning 0 on 0 kills ***
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.
Powered by blists - more mailing lists