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]

Date:	Thu, 13 Aug 2015 14:55:50 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.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

Let me first say that CLONE_SIGHAND must die, I think ;) and perhaps
even sighand_struct... I am wondering if we can add something like

	if ((clone_flags & (CLONE_THREAD | CLONE_SIGHAND)) == CLONE_SIGHAND)
		pr_info("You are crazy, please report this to lkml\n");

into copy_process().

On 08/12, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> > On 08/11, Eric W. Biederman wrote:
> >>  	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.

current_is_single_threaded() checks task->signal->live at the start,
so there is no optimization. But I won't argue, this doesn't hurt.

> >>  	/*
> >> +	 * 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.

OK,

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

See below,

> CLONE_SIGHAND means that you want a struct sighand_struct with a count
> of 1.

This is (almost) true,

> Nothing about a sighand_struct with a count of 1 implies or
> requires mm_users == 1.  clone can quite happily create those.

See

	if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))

in copy_process(). So if you have a shared sighand_struct, your ->mm
is also shared, current_is_single_threaded() will notice this.

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

How that? clone(VM | SIGHAND) will share ->sighand and increment its
count.

> unshare(CLONE_SIGHAND) requests a sighand_struct with
> a count of 1.

Exactly, that is why it is wrong to succeed.

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

Oh, I do not think we should check sighand->count. This can lead to
the same problem we have with the current current->mm->mm_users check.

Most probably today nobody increments sighand->count (I didn't even
try to verify). But this is possible, and I saw the code which did
this to pin ->sighand...

Oleg.

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