From: Thomas Gleixner <[email protected]>
The upcoming support for dynamically enabled FPU features on x86 requires
an architecture specific sanity check and serialization of the store to
task::sas_ss_size. The check is required to ensure that:
- Enabling of a dynamic feature, which changes the sigframe size fits
into an enabled sigaltstack
- Installing a too small sigaltstack after a dynamic feature has been
added is not possible.
It needs serialization to prevent race conditions of all sorts in the
feature enable code as that has to walk the thread list of the process.
Add the infrastructure in form of a config option and provide empty stubs
for architectures which do not need that.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Eric W. Biederman <[email protected]>
---
arch/Kconfig | 3 +++
include/linux/signal.h | 6 ++++++
kernel/signal.c | 35 +++++++++++++++++++++++++++++------
3 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 8df1c7102643..af5cf3009b4f 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1288,6 +1288,9 @@ config ARCH_HAS_ELFCORE_COMPAT
config ARCH_HAS_PARANOID_L1D_FLUSH
bool
+config DYNAMIC_SIGFRAME
+ bool
+
source "kernel/gcov/Kconfig"
source "scripts/gcc-plugins/Kconfig"
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 3f96a6374e4f..7d34105e20c6 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -464,6 +464,12 @@ int __save_altstack(stack_t __user *, unsigned long);
unsafe_put_user(t->sas_ss_size, &__uss->ss_size, label); \
} while (0);
+#ifdef CONFIG_DYNAMIC_SIGFRAME
+bool sigaltstack_size_valid(size_t ss_size);
+#else
+static inline bool sigaltstack_size_valid(size_t size) { return true; }
+#endif /* !CONFIG_DYNAMIC_SIGFRAME */
+
#ifdef CONFIG_PROC_FS
struct seq_file;
extern void render_sigset_t(struct seq_file *, const char *, sigset_t *);
diff --git a/kernel/signal.c b/kernel/signal.c
index 952741f6d0f9..9278f5291ed6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -4151,11 +4151,29 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
return 0;
}
+#ifdef CONFIG_DYNAMIC_SIGFRAME
+static inline void sigaltstack_lock(void)
+ __acquires(¤t->sighand->siglock)
+{
+ spin_lock_irq(¤t->sighand->siglock);
+}
+
+static inline void sigaltstack_unlock(void)
+ __releases(¤t->sighand->siglock)
+{
+ spin_unlock_irq(¤t->sighand->siglock);
+}
+#else
+static inline void sigaltstack_lock(void) { }
+static inline void sigaltstack_unlock(void) { }
+#endif
+
static int
do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
size_t min_ss_size)
{
struct task_struct *t = current;
+ int ret = 0;
if (oss) {
memset(oss, 0, sizeof(stack_t));
@@ -4179,19 +4197,24 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
ss_mode != 0))
return -EINVAL;
+ sigaltstack_lock();
if (ss_mode == SS_DISABLE) {
ss_size = 0;
ss_sp = NULL;
} else {
if (unlikely(ss_size < min_ss_size))
- return -ENOMEM;
+ ret = -ENOMEM;
+ if (!sigaltstack_size_valid(ss_size))
+ ret = -ENOMEM;
}
-
- t->sas_ss_sp = (unsigned long) ss_sp;
- t->sas_ss_size = ss_size;
- t->sas_ss_flags = ss_flags;
+ if (!ret) {
+ t->sas_ss_sp = (unsigned long) ss_sp;
+ t->sas_ss_size = ss_size;
+ t->sas_ss_flags = ss_flags;
+ }
+ sigaltstack_unlock();
}
- return 0;
+ return ret;
}
SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss)
--
2.17.1
On Oct 21, 2021, at 15:55, Bae, Chang Seok <[email protected]> wrote:
>
> Cc: Oleg Nesterov <[email protected]>
Hi Oleg,
Apologies on my typo -- s/ole/oleg/
The original message can be found:
https://lore.kernel.org/lkml/[email protected]/
Thanks,
Chang
"Chang S. Bae" <[email protected]> writes:
> From: Thomas Gleixner <[email protected]>
>
> The upcoming support for dynamically enabled FPU features on x86 requires
> an architecture specific sanity check and serialization of the store to
> task::sas_ss_size. The check is required to ensure that:
>
> - Enabling of a dynamic feature, which changes the sigframe size fits
> into an enabled sigaltstack
>
> - Installing a too small sigaltstack after a dynamic feature has been
> added is not possible.
>
> It needs serialization to prevent race conditions of all sorts in the
> feature enable code as that has to walk the thread list of the process.
>
> Add the infrastructure in form of a config option and provide empty stubs
> for architectures which do not need that.
Last I looked Al Viro was doing a lot of work on sigframes, adding him
to the cc.
That said description in the patch is very sorely lacking.
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.
Second the reason that 2K is not enough mentioned. The current value of
MINSIGSTKSZ on x86 is 2K.
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.
Forth the sigframe size on x86 is already dynamic and is already
computed by get_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.
Thank you,
Eric
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Chang S. Bae <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> ---
> arch/Kconfig | 3 +++
> include/linux/signal.h | 6 ++++++
> kernel/signal.c | 35 +++++++++++++++++++++++++++++------
> 3 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8df1c7102643..af5cf3009b4f 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1288,6 +1288,9 @@ config ARCH_HAS_ELFCORE_COMPAT
> config ARCH_HAS_PARANOID_L1D_FLUSH
> bool
>
> +config DYNAMIC_SIGFRAME
> + bool
> +
> source "kernel/gcov/Kconfig"
>
> source "scripts/gcc-plugins/Kconfig"
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 3f96a6374e4f..7d34105e20c6 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -464,6 +464,12 @@ int __save_altstack(stack_t __user *, unsigned long);
> unsafe_put_user(t->sas_ss_size, &__uss->ss_size, label); \
> } while (0);
>
> +#ifdef CONFIG_DYNAMIC_SIGFRAME
> +bool sigaltstack_size_valid(size_t ss_size);
> +#else
> +static inline bool sigaltstack_size_valid(size_t size) { return true; }
> +#endif /* !CONFIG_DYNAMIC_SIGFRAME */
> +
> #ifdef CONFIG_PROC_FS
> struct seq_file;
> extern void render_sigset_t(struct seq_file *, const char *, sigset_t *);
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 952741f6d0f9..9278f5291ed6 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -4151,11 +4151,29 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
> return 0;
> }
>
> +#ifdef CONFIG_DYNAMIC_SIGFRAME
> +static inline void sigaltstack_lock(void)
> + __acquires(¤t->sighand->siglock)
> +{
> + spin_lock_irq(¤t->sighand->siglock);
> +}
> +
> +static inline void sigaltstack_unlock(void)
> + __releases(¤t->sighand->siglock)
> +{
> + spin_unlock_irq(¤t->sighand->siglock);
> +}
> +#else
> +static inline void sigaltstack_lock(void) { }
> +static inline void sigaltstack_unlock(void) { }
> +#endif
> +
> static int
> do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
> size_t min_ss_size)
> {
> struct task_struct *t = current;
> + int ret = 0;
>
> if (oss) {
> memset(oss, 0, sizeof(stack_t));
> @@ -4179,19 +4197,24 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
> ss_mode != 0))
> return -EINVAL;
>
> + sigaltstack_lock();
> if (ss_mode == SS_DISABLE) {
> ss_size = 0;
> ss_sp = NULL;
> } else {
> if (unlikely(ss_size < min_ss_size))
> - return -ENOMEM;
> + ret = -ENOMEM;
> + if (!sigaltstack_size_valid(ss_size))
> + ret = -ENOMEM;
> }
> -
> - t->sas_ss_sp = (unsigned long) ss_sp;
> - t->sas_ss_size = ss_size;
> - t->sas_ss_flags = ss_flags;
> + if (!ret) {
> + t->sas_ss_sp = (unsigned long) ss_sp;
> + t->sas_ss_size = ss_size;
> + t->sas_ss_flags = ss_flags;
> + }
> + sigaltstack_unlock();
> }
> - return 0;
> + return ret;
> }
>
> SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss)
Hi Eric,
On Oct 22, 2021, at 08:20, Eric W. Biederman <[email protected]> wrote:
>
> "Chang S. Bae" <[email protected]> writes:
>
>> From: Thomas Gleixner <[email protected]>
>>
>> The upcoming support for dynamically enabled FPU features on x86 requires
>> an architecture specific sanity check and serialization of the store to
>> task::sas_ss_size. The check is required to ensure that:
>>
>> - Enabling of a dynamic feature, which changes the sigframe size fits
>> into an enabled sigaltstack
>>
>> - Installing a too small sigaltstack after a dynamic feature has been
>> added is not possible.
>>
>> It needs serialization to prevent race conditions of all sorts in the
>> feature enable code as that has to walk the thread list of the process.
>>
>> Add the infrastructure in form of a config option and provide empty stubs
>> for architectures which do not need that.
>
> Last I looked Al Viro was doing a lot of work on sigframes, adding him
> to the cc.
>
>
> That said description in the patch is very sorely lacking.
Will update the changelog though, let me clarify what you pointed out here
at first.
> 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.
Later in this series, a new x86-specific prctl() is introduced so that an
application may ask permission to use dynamic states. It means the sigframe
size is also dynamic. Besides that, in the implemented mechanism [1]:
"Task asks for permission for a facility and kernel checks whether that's
supported. If supported it does:
...
3) Validate that no task has a sigaltstack installed
which is smaller than the resulting sigframe size
...
"
The new sigframe size out of new permission is validated via each thread’s
altstack size. Accessing each task::sas_ss_size can be racy with
sigaltstack(). So, it is protected by sighand lock. (3) looks like this in a
nutshell:
spin_lock_irq(¤t->sighand->siglock);
...
int validate_sigaltstack(unsigned int usize) {
struct task_struct *thread, *leader = current->group_leader;
unsigned long framesize = get_sigframe_size();
...
for_each_thread(leader, thread) {
if (thread->sas_ss_size && thread->sas_ss_size < framesize)
return -ENOSPC;
}
...
}
...
spin_unlock_irq(¤t->sighand->siglock);
> Second the reason that 2K is not enough mentioned. The current value of
> MINSIGSTKSZ on x86 is 2K.
The MINSIGSTKSZ constant is 2K but this is already too small in x86 with
AVX512. Increasing this might break binaries that were already compiled with
the old value. They used to work because their sigalstack was never used.
> 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.
Now, there is an effort to redefine MINSIGSTKSZ as a dynamic value.
The latest glibc v2.34 has this [2]:
"Add _SC_MINSIGSTKSZ and _SC_SIGSTKSZ. When _DYNAMIC_STACK_SIZE_SOURCE
or _GNU_SOURCE are defined, MINSIGSTKSZ and SIGSTKSZ are no longer
constant on Linux. MINSIGSTKSZ is redefined to sysconf(_SC_MINSIGSTKSZ)
and SIGSTKSZ is redefined to sysconf (_SC_SIGSTKSZ). This supports
dynamic sized register sets for modern architectural features like
Arm SVE."
> Forth the sigframe size on x86 is already dynamic and is already
> computed by get_sigframe_size.
Also, the x86 kernel supports exporting it via the AT_MINSIGSTKSZ aux vector
[4] since v5.14. I had developed the code with H.J. who authored the glibc
code [3].
> So can we please please please have a better description of what
> is going on and the trade offs that are being made.
Okay, but I think the dynamic MINSIGSTKSZ is not what this patch does, no?
Maybe the task::sas_ss_size part needs more clarity though.
Thanks,
Chang
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://sourceware.org/pipermail/libc-alpha/2021-August/129718.html
[3] https://sourceware.org/git/?p=glibc.git;a=commit;h=6c57d320484988e87e446e2e60ce42816bf51d53
[4] https://lore.kernel.org/lkml/[email protected]/
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
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 1bdda24c4af64cd2d65dec5192ab624c5fee7ca0
Gitweb: https://git.kernel.org/tip/1bdda24c4af64cd2d65dec5192ab624c5fee7ca0
Author: Thomas Gleixner <[email protected]>
AuthorDate: Thu, 21 Oct 2021 15:55:05 -07:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 26 Oct 2021 10:15:12 +02:00
signal: Add an optional check for altstack size
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 a future patch called
x86/arch_prctl: Add controls for dynamic XSTATE components
[dhansen: commit message. ]
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/Kconfig | 3 +++
include/linux/signal.h | 6 ++++++
kernel/signal.c | 35 +++++++++++++++++++++++++++++------
3 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 8df1c71..af5cf30 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1288,6 +1288,9 @@ config ARCH_HAS_ELFCORE_COMPAT
config ARCH_HAS_PARANOID_L1D_FLUSH
bool
+config DYNAMIC_SIGFRAME
+ bool
+
source "kernel/gcov/Kconfig"
source "scripts/gcc-plugins/Kconfig"
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 3f96a63..7d34105 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -464,6 +464,12 @@ int __save_altstack(stack_t __user *, unsigned long);
unsafe_put_user(t->sas_ss_size, &__uss->ss_size, label); \
} while (0);
+#ifdef CONFIG_DYNAMIC_SIGFRAME
+bool sigaltstack_size_valid(size_t ss_size);
+#else
+static inline bool sigaltstack_size_valid(size_t size) { return true; }
+#endif /* !CONFIG_DYNAMIC_SIGFRAME */
+
#ifdef CONFIG_PROC_FS
struct seq_file;
extern void render_sigset_t(struct seq_file *, const char *, sigset_t *);
diff --git a/kernel/signal.c b/kernel/signal.c
index 952741f..9278f52 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -4151,11 +4151,29 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
return 0;
}
+#ifdef CONFIG_DYNAMIC_SIGFRAME
+static inline void sigaltstack_lock(void)
+ __acquires(¤t->sighand->siglock)
+{
+ spin_lock_irq(¤t->sighand->siglock);
+}
+
+static inline void sigaltstack_unlock(void)
+ __releases(¤t->sighand->siglock)
+{
+ spin_unlock_irq(¤t->sighand->siglock);
+}
+#else
+static inline void sigaltstack_lock(void) { }
+static inline void sigaltstack_unlock(void) { }
+#endif
+
static int
do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
size_t min_ss_size)
{
struct task_struct *t = current;
+ int ret = 0;
if (oss) {
memset(oss, 0, sizeof(stack_t));
@@ -4179,19 +4197,24 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
ss_mode != 0))
return -EINVAL;
+ sigaltstack_lock();
if (ss_mode == SS_DISABLE) {
ss_size = 0;
ss_sp = NULL;
} else {
if (unlikely(ss_size < min_ss_size))
- return -ENOMEM;
+ ret = -ENOMEM;
+ if (!sigaltstack_size_valid(ss_size))
+ ret = -ENOMEM;
}
-
- t->sas_ss_sp = (unsigned long) ss_sp;
- t->sas_ss_size = ss_size;
- t->sas_ss_flags = ss_flags;
+ if (!ret) {
+ t->sas_ss_sp = (unsigned long) ss_sp;
+ t->sas_ss_size = ss_size;
+ t->sas_ss_flags = ss_flags;
+ }
+ sigaltstack_unlock();
}
- return 0;
+ return ret;
}
SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss)