linux-kernel - Re: [PATCH 1/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 12:27:48 +0100
From:   Petr Skocik <pskocik@...il.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Marco Elver <elver@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] Fix kill(-1,s) returning 0 on 0 kills

On 11/23/22 11:30, Oleg Nesterov wrote:
> On 11/22, Petr Skocik wrote:
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -1600,20 +1600,18 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
>>   		ret = __kill_pgrp_info(sig, info,
>>   				pid ? find_vpid(-pid) : task_pgrp(current));
>>   	} else {
>> -		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*/
> The patch looks good to me, and it also simplifies the code.
>
> But I fail to understand the /*either all 0 or all -EINVAL*/ comment above..
>
> Oleg.
>

Thanks. The comment is explained in my reply to Kees Cook: 
https://lkml.org/lkml/2022/11/22/1327.
I felt like making it because without it to me it suspiciously looks 
like the
`if ( err != -EPERM) ret = err;` (or `if ( err != -EPERM) retval = err;` 
in the original) could be masking
a non-EPERM failure with a later success, but it isn't because in this 
context, all the non-EPERM return vals should either ALL be 0 or ALL be 
-EINVAL.
The original code seems to make this assumption too, although implicitly.
Perhaps this should be asserted somehow (WARN_ON?).

If it couldn't be assumed, I'd imagine you'd want to guard against such 
masking

         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);
                 if (err != -EPERM){
                     ++count;
                     if ( err < 0 ) /*retval is 0 to start with and set 
to negatives only*/
                         retval = err;
                 }
             }
         }
         ret = count ? retval : -ESRCH;

Regards,
Petr Skocik