linux-kernel - Re: [PATCH 0/1] *** Fix kill(-1,s) returning 0 on 0 kills ***

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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.