lkml.org 
[lkml]   [2022]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 0/1] *** Fix kill(-1,s) returning 0 on 0 kills ***
From
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.

\
 
 \ /
  Last update: 2022-11-23 00:02    [W:0.097 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site