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

[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?
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.