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: <20160201160625.GA18276@redhat.com>
Date:	Mon, 1 Feb 2016 17:06:25 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Stas Sergeev <stsp@...t.ru>
Cc:	Linux kernel <linux-kernel@...r.kernel.org>,
	linux-api@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"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>
Subject: Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas
 within sighandler

Honestly, I am not sure I understand what this patch does and why, and it is
white space damaged, please fix.

On 01/31, Stas Sergeev wrote:
>
> linux implements the sigaltstack() in a way that makes it impossible to
> use with swapcontext(). Per the man page, sigaltstack is allowed to return
> EPERM if the process is altering its sigaltstack while running on
> sigaltstack.
> This is likely needed to consistently return oss->ss_flags, that indicates
> whether the process is being on sigaltstack or not.
> Unfortunately, linux takes that permission to return EPERM too literally:
> it returns EPERM even if you don't want to change to another sigaltstack,
> but only want to temporarily disable sigaltstack with SS_DISABLE.
> You can't use swapcontext() without disabling sigaltstack first, or the
> stack will be re-used and overwritten by a subsequent signal.

So iiuc you want to switch the stack from the signal handler running on the
alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt
the alt stack in between, right?

Perhaps you can update the changelog to explain why do we want this change.


> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
>  {
>      if (!current->sas_ss_size)
>          return SS_DISABLE;
> -
> -    return on_sig_stack(sp) ? SS_ONSTACK : 0;
> +    if (on_sig_stack(sp))
> +        return SS_ONSTACK;
> +    if (current->sas_ss_flags == SS_DISABLE)
> +        return SS_DISABLE;
> +    return 0;

So this always return SS_ONSTACK if on_sig_stack(), see below.

> +        onsigstack = on_sig_stack(sp);
> +        if (ss_size == 0) {
> +            switch (ss_flags) {
> +            case 0:
> +                error = -EPERM;
> +                if (onsigstack)
> +                    goto out;
> +                current->sas_ss_sp = 0;
> +                current->sas_ss_size = 0;
> +                current->sas_ss_flags = SS_DISABLE;
> +                break;
> +            case SS_ONSTACK:
> +                /* re-enable previously disabled sas */
> +                error = -EINVAL;
> +                if (current->sas_ss_size == 0)
> +                    goto out;
> +                break;
> +            default:
> +                break;
> +            }

and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
even if on_sig_stack().

So the sequence is

	// running on alt stack

	sigaltstack(SS_DISABLE);

	temporary_run_on_another_stack();

	sigaltstack(SS_ONSTACK);

and SS_DISABLE saves us from another SA_ONSTACK signal, right?

But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
your changes (afaics), it won't pick the alt stack after SS_DISABLE.

However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
uc_stack->ss_flags, and after return from signal handler restore_altstack() will
enable alt stack again?

Oleg.