linux-kernel - Re: [PATCH 01/23] signal: Add an optional check for altstack size

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: <a82349a1-de71-2d68-fe62-32b395d523ec@intel.com>
Date:   Fri, 22 Oct 2021 15:51:00 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
        dave.hansen@...ux.intel.com, arjan@...ux.intel.com,
        ravi.v.shankar@...el.com, Oleg Nesterov <ole@...hat.com>,
        Al Viro <viro@...IV.linux.org.uk>
Subject: Re: [PATCH 01/23] signal: Add an optional check for altstack size

Hi Eric,

> First the reason for the new locking is not really explained, it talks
> about serialization but it does not talk about what is protected.
> Especially given that the signal delivery code already has to check if
> the signal frame on the stack when pushing a new signal I don't
> understand what the code is trying to prevent.

Yeah, the basic idea is to ensure that there are no races between
do_sigaltstack() and enabling the new "dynamic features".

> Third the issues with modifying the userspace ABI are not discussed. 
> Frankly that is a pretty big consideration.  MINSIGSTKSZ is exported
> to userspace and userspace fundamentally needs to allocate the
> alternate signal frame.
Agreed.  This is what we settled on to both respect old programs that
have MINSIGSTKSZ=2k compiled in *and* support new ones that need bigger
stacks.

> Forth the sigframe size on x86 is already dynamic and is already
> computed by get_sigframe_size.

Right.  This is much more about making the altstack size checks dynamic
than the sigframe size.

> So can we please please please have a better description of what
> is going on and the trade offs that are being made.

I've got a suggested replacement changelog below.  Please let me know if
it clarifies things, or leaves anything out.

How would this be a better subject?

	signal: Add optional dynamic altstack size checks

And this for a changelog?

--

New x86 FPU features will be very large, requiring ~10k of stack in
signal handlers.  These new features require a new approach called
"dynamic features".

The kernel currently tries to ensure that altstacks are reasonably
sized.  Right now, on x86, sys_sigaltstack() requires a size of >=2k.
However, that 2k is a constant.  Simply raising that 2k requirement to
>10k for the new features would break existing apps which have a
compiled-in size of 2k.

Instead of universally enforcing a larger stack, prohibit a process from
using dynamic features without properly-sized altstacks.  This must be
enforced in two places:

 * A dynamic feature can not be enabled without an large-enough altstack
   for each process thread.
 * Once a dynamic feature is enabled, any request to install a too-small
   altstack will be rejected

The dynamic feature enabling code must examine each thread in a process
to ensure that the altstacks are large enough.  Add a new lock
(sigaltstack_lock()) to ensure that threads can not race and change
their altstack after being examined.

Add the infrastructure in form of a config option and provide empty
stubs for architectures which do not need dynamic altstack size checks.

This implementation will be fleshed out for x86 in:

	x86/arch_prctl: Add controls for dynamic XSTATE components