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]

Message-ID: <CALCETrU2u7h98oqtMcgvU54j21-bpTfBXUEJNQO9r1w5FHc-HQ@mail.gmail.com>
Date:	Sun, 31 Jan 2016 14:44:16 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Stas Sergeev <stsp@...t.ru>
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

On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp@...t.ru> wrote:
> 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.

As long as the kernel has a concept of a programmed but disabled
sigaltstack, something is confused when there is more than one
user-created inactive sigaltstack.  So I don't see why you want the
kernel to remember about disabled altstacks at all.

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

Why would there only be one per thread?

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

That's only the case if swapcontext is implemented using rt_sigreturn.  Is it?

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

...which is why I suggested SS_FORCE in the first place.

--Andy