linux-kernel - Re: [PATCH 1/2] signals: kill(-1) should only signal processes in the same namespace

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] [day] [month] [year] [list]

Date:	Wed, 23 Jul 2008 18:09:40 +0200 (CEST)
From:	"Daniel Hokka Zakrisson" <daniel@...ac.com>
To:	"Oleg Nesterov" <oleg@...sign.ru>
Cc:	linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, ebiederm@...ssion.com,
	xemul@...nvz.org, akpm@...ux-foundation.org
Subject: Re: [PATCH 1/2] signals: kill(-1) should only signal processes in 
     the same namespace

Oleg Nesterov wrote:
> On 07/17, Daniel Hokka Zakrisson wrote:
>>
>> +int task_in_pid_ns(struct task_struct *tsk, struct pid_namespace *ns)
>> +{
>> +	struct pid *pid = task_pid(tsk);
>> +
>> +	if (!pid)
>> +		return 0;
>> +
>> +	if (pid->level < ns->level)
>> +		return 0;
>> +
>> +	if (pid->numbers[ns->level].ns != ns)
>> +		return 0;
>> +
>> +	return 1;
>> +}
>> +
>>  static __init int pid_namespaces_init(void)
>>  {
>>  	pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 6c0958e..93713a5 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
>> siginfo *info, int pid)
>>  		struct task_struct * p;
>>
>>  		for_each_process(p) {
>> -			if (p->pid > 1 && !same_thread_group(p, current)) {
>> +			if (p->pid > 1 && !same_thread_group(p, current) &&
>> +			    task_in_pid_ns(p, current->nsproxy->pid_ns)) {
>>  				int err = group_send_sig_info(sig, info, p);
>>  				++count;
>>  				if (err != -EPERM)
>
> Do we really need all these complications? Afaics, we can make
> a simpler patch,
>
> 	--- kernel/signal.c
> 	+++ kernel/signal.c
> 	@@ -1136,7 +1136,7 @@ static int kill_something_info(int sig,
> 			struct task_struct * p;
>
> 			for_each_process(p) {
> 	-			if (p->pid > 1 && !same_thread_group(p, current)) {
> 	+			if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) {
> 					int err = group_send_sig_info(sig, info, p);
> 					++count;
> 					if (err != -EPERM)
>
>
> task_pid_vnr(p) returns 0 if "p" is not visible from the current's
> namespace. "> 1" ensures we don't kill the child reaper as well.
>
> No?
>
> Oleg.

You are absolutely right, that is sufficient and much cleaner.

-- 
Daniel Hokka Zakrisson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/