linux-kernel - Re: [PATCH 1/2] unshare: Unsharing a thread does not require unsharing a vm

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]

Message-ID: <87614k73mo.fsf@x220.int.ebiederm.org>
Date:	Wed, 12 Aug 2015 13:39:27 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	"Kirill A. Shutemov" <kirill@...temov.name>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kees Cook <keescook@...omium.org>,
	David Howells <dhowells@...hat.com>,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>,
	Vladimir Davydov <vdavydov@...allels.com>,
	Ricky Zhou <rickyz@...omium.org>,
	Julien Tinnes <jln@...gle.com>
Subject: Re: [PATCH 1/2] unshare: Unsharing a thread does not require unsharing a vm

Oleg Nesterov <oleg@...hat.com> writes:

> On 08/11, Eric W. Biederman wrote:
>>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1866,13 +1866,17 @@ static int check_unshare_flags(unsigned long unshare_flags)
>>  				CLONE_NEWUSER|CLONE_NEWPID))
>>  		return -EINVAL;
>>  	/*
>> -	 * Not implemented, but pretend it works if there is nothing to
>> -	 * unshare. Note that unsharing CLONE_THREAD or CLONE_SIGHAND
>> -	 * needs to unshare vm.
>> +	 * Not implemented, but pretend it works if there is nothing
>> +	 * to unshare.  Note that unsharing the address space or the
>> +	 * signal handlers also need to unshare the signal queues (aka
>> +	 * CLONE_THREAD).
>>  	 */
>>  	if (unshare_flags & (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)) {
>> -		/* FIXME: get_task_mm() increments ->mm_users */
>> -		if (atomic_read(&current->mm->mm_users) > 1)
>> +		if (!thread_group_empty(current))
>> +			return -EINVAL;
>> +	}
>> +	if (unshare_flags & CLONE_VM) {
>> +		if (!current_is_single_threaded())
>>  			return -EINVAL;
>>  	}
>
> OK, but then you can remove "| CLONE_VM" from the previous check...

As an optimization, but I don't think anything cares enough for the
optimization to be worth the confusion.

>> @@ -1941,16 +1945,16 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
>>  	if (unshare_flags & CLONE_NEWUSER)
>>  		unshare_flags |= CLONE_THREAD | CLONE_FS;
>>  	/*
>> -	 * If unsharing a thread from a thread group, must also unshare vm.
>> -	 */
>> -	if (unshare_flags & CLONE_THREAD)
>> -		unshare_flags |= CLONE_VM;
>
> OK,
>
>>  	/*
>> +	 * If unsharing a signal handlers, must also unshare the signal queues.
>> +	 */
>> +	if (unshare_flags & CLONE_SIGHAND)
>> +		unshare_flags |= CLONE_THREAD;
>
> This looks unnecessary, check_unshare_flags() checks "THREAD | SIGHAND".
> And to me the comment looks misleading although I won't argue.

I absolutely can not understand this code if we jump 5 steps ahead
and optimize out the individual dependencies, and try for a flattened
dependency tree instead.  I can validate the individual dependencies
from first principles.

If we jump several steps ahead I can not validate the individual
dependencies.  

It really is important to say if you want your own private struct
sighand_struct, you also need to have your own private struct
signal_struct.

> And in fact this doesn't look exactly right, or I am totally confused.
> Shouldn't we do
>
> 	if (unshare_flags & CLONE_SIGHAND)
> 		unshare_flags |= CLONE_VM;

Nope.  The backward definitions of the flags in unshare has gotten you.
CLONE_SIGHAND means that you want a struct sighand_struct with a count
of 1.  Nothing about a sighand_struct with a count of 1 implies or
requires mm_users == 1.  clone can quite happily create those.

> ? Or change check_unshare_flags()...
>
> Otherwise suppose that a single threaded process does clone(VM | SIGHAND)
> and (say) child does sys_unshare(SIGHAND). This will wrongly succeed
> afaics.

Why would it be wrong to succeed in that case?  struct sighand_struct
has a count of 1.  unshare(CLONE_SIGHAND) requests a sighand_struct with
a count of 1.

I expect part of the confusion is the code in unshare has been wrongly
requiring an unshared vm to support a sighand_struct with a count of 1
since the day the code was merged.

Ugh. This patch has a bug where we don't check for sighand->count == 1.

clone(VM)  ---> mm_users = 2 sighand->count == 1 signal->live == 1

clone(VM|SIGHAND) --> mm_users = 2 sighand->count == 2 signal->live == 1

unshare(SIGHAND) needs to guarantee that when it returns sighand->count == 1.
So unshare(SIGHAND) needs to test for sighand->count == 1.

Ugh.  Untangling this ancient mess is a pain.  One more pass at this
patch it seems.

Eric
--
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/