linux-kernel - Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

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:	Mon, 1 Feb 2016 01:36:48 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Linux kernel <linux-kernel@...r.kernel.org>,
	Linux API <linux-api@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Amanieu d'Antras <amanieu@...il.com>,
	Richard Weinberger <richard@....at>, Tejun Heo <tj@...nel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Jason Low <jason.low2@...com>,
	Heinrich Schuchardt <xypron.glpk@....de>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
	Josh Triplett <josh@...htriplett.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Aleksa Sarai <cyphar@...har.com>,
	Paul Moore <pmoore@...hat.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Vladimir Davydov <vdavydov@...allels.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas
 within sighandler

31.01.2016 23:11, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@...t.ru> wrote:
>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>> Also, consider a use case like yours but with *two* contexts that use
>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>> signal, temporarily disable, then swapcontext to B, which tries to
>>> re-enable its own sigaltstack, then everything gets confusing with
>>> your patch, because, with your patch, the kernel is only tracking one
>>> temporarily disabled sigaltstack.
>> Of course the good practice is to set the sigaltstack
>> before creating the contexts. Then the above scenario
>> should involve switching between 2 signal handlers to get
>> into troubles. I think the scenario with switching between
>> 2 signal handlers is very-very unrealistic.
> Why is it so unrealistic?  You're already using swapcontext, which
> means you're doing something like userspace threads (although I
> imagine that one of your thread-like things is DOS, but still), and,
> to me, that suggests that the kernel interface should be agnostic as
> to how many thread-like thinks are alive.
But you only get into troubles when you switch between 2
_active signal handlers_, rather than between 2 normal contexts,
or between 2 normal context and 1 sighandler.
So I am probably misunderstanding the scenario you describe.
Without 2 sighandlers that are active at the same time and you
switch between them, how would you get into troubles?
You say "then swapcontext to B, which tries to re-enable its own 
sigaltstack"
but there can be only one sigaltstack per thread, so I am quite
sure by re-enabling "its own sigaltstack" it will still do the right
thing.

> With your patch, where the kernel remembers that you have a
> temporarily disabled altstack, you can't swap out your context on one
> kernel thread and swap it in on another,
I can if I always set up a new stack rather than re-enable, right?
But yes, this is a problem that counts.

>   and you can't have two
> different contexts that get used on the same thread.
I don't think this is the problem because only the signal handler
should re-enable the sigaltstack, and I don't think we really should
switch between 2 active signal handlers. And even if we did, there
can be only one sigaltstack per thread, so it will re-enable always
the right stack (there is only one).

> ISTM it would be simpler if you did:
>
> sigaltstack(disable, force)
> swapcontext() to context using sigaltstack
> sigaltstack(set new altstack)
>
> and then later
>
> sigaltstack(disable, force)  /* just in case.  save old state, too. */
> swapcontext() to context not using sigaltstack
> sigaltstack(set new altstack)
In the real world you don't even need sigaltstack(set new altstack)
because uc_stack does this for you on rt_sigreturn. It is only my
test-case that does so.

> If it would be POSIX compliant to allow SS_DISABLE to work even if on
> the altstack even without a new flag (which is what you're
> suggesting), then getting rid of the temporary in-kernel state would
> considerably simplify this patch series.  Just skip the -EPERM check
> in the disable path.
Yes, that's why I was suggesting to just remove the EPERM
check initially. We can still do exactly that. The only problem I
can see with removing EPERM is that it would be hard to emulate
the old behaviour if need be. For example if glibc want to return
EPERM behaviour, it will have problems doing so because oss->ss_flags
doesn't say if we are on a sigaltstack and there is no other way
to find out.