linux-kernel - Re: [PATCH v2] 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 11:01:46 -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 v2] unshare: Unsharing a thread does not require unsharing a vm

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

> On 08/12, Eric W. Biederman wrote:
>>
>> +	if (unshare_flags & (CLONE_SIGHAND | CLONE_VM)) {
>> +		if (atomic_read(&current->sighand->count) > 1)
>> +			return -EINVAL;
>> +	}
>
> I am still not sure we want this... please the the previous email.

Reading your other email I did not see why you thought this check was
unnecessary.

> But perhaps I missed something.

In short:
clone(VM) --> mm_users > 1 && sighand_struct->count == 1
followed by:
unshare(SIGHAND)
the unshare should succeed.

Meanwhile:
clone(VM|SIGHAND) --> mm_users > 1 && sighand_struct->count > 1
followed by:
unshare(SIGHAND)
the unshare should fail.

I actually tested both of these cases and my patch works properly.

Not that I expect that there is anyone actually calling unshare(SIGHAND)
but unless we figure out how to remove the code, the code should
function correctly.  If for no other reason than to not confuse people
reading and maintaining the code.

Further I have audited the callers and we don't have anyone playing
games with sighand->count.  There is an implementation of unsharing the
sighand_struct in dethread in fs/exec.c that relies on this.

Other possible tests such as current_is_single_threaded() and
thread_group_empty() fail at the wrong times to be used.

So I think it is clear that testing for a private sighand_struct is
necessaring and testing sighand->count is a perfectly fine test.

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/