Messages in this thread Patch in this message | | | Date | Wed, 23 Nov 2022 00:01:39 +0100 | Subject | Re: [PATCH 0/1] *** Fix kill(-1,s) returning 0 on 0 kills *** | From | 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.
| |