[PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Chang S. Bae]

Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
use by all architectures with sigaltstack(2). Over time, the hardware state
size grew, but these constants did not evolve. Today, literal use of these
constants on several architectures may result in signal stack overflow, and
thus user data corruption.

A few years ago, the ARM team addressed this issue by establishing
getauxval(AT_MINSIGSTKSZ). This enables the kernel to supply at runtime
value that is an appropriate replacement on the current and future
hardware.

Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
size to userspace via auxv").

Also, include a documentation to describe x86-specific auxiliary vectors.

Reported-by: Florian Weimer <fweimer@redhat.com>
Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: x86@kernel.org
Cc: libc-alpha@sourceware.org
Cc: linux-arch@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
---
Changes from v5:
* Added a documentation.
---
 Documentation/x86/elf_auxvec.rst   | 56 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/elf.h         |  4 +++
 arch/x86/include/uapi/asm/auxvec.h |  4 +--
 arch/x86/kernel/signal.c           |  5 +++
 4 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/x86/elf_auxvec.rst

diff --git a/Documentation/x86/elf_auxvec.rst b/Documentation/x86/elf_auxvec.rst
new file mode 100644
index 000000000000..751c552c4048
--- /dev/null
+++ b/Documentation/x86/elf_auxvec.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==================================
+x86-specific ELF Auxiliary Vectors
+==================================
+
+This document describes the semantics of the x86 auxiliary vectors.
+
+1. Introduction
+---------------
+
+ELF Auxiliary vectors enable the kernel to efficiently provide
+configuration specific parameters to userspace. In this example, a program
+allocates an alternate stack based on the kernel-provided size.
+
+   #include <sys/auxv.h>
+   #include <elf.h>
+
+   #ifndef AT_MINSIGSTKSZ
+   #define AT_MINSIGSTKSZ	51
+   #endif
+
+   stack_t ss;
+   int err;
+
+   ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+   ss.ss_sp = malloc(ss.ss_size);
+   ...
+
+   err = sigaltstack(&ss, NULL);
+   ...
+
+
+2. The exposed auxiliary vectors
+---------------------------------
+
+AT_SYSINFO
+    The entry point to the system call function the virtual Dynamic Shared
+    Object (vDSO), not exported on 64-bit.
+
+AT_SYSINFO_EHDR
+    The start address of the page containing vDSO.
+
+AT_MINSIGSTKSZ
+    The minimum stack size required to deliver a signal. It is a calculated
+    sigframe size based on the largest possible user context. When programs
+    use sigaltstack() to provide alternate signal stack, that stack must be
+    at least the size to function properly on this hardware. Note that this
+    is a minimum of the kernel to correctly get to the signal handler.
+    Additional space must be added to handle objects pushed onto the stack
+    by the signal handlers, as well as for nested signal delivery.
+
+    The purpose of this parameter is to accommodate the different stack
+    sizes required by different hardware configuration. E.g., the x86
+    system supporting the Advanced Vector Extension needs at least 8KB more
+    than the one without it.
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 66bdfe838d61..cd10795c178e 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -312,6 +312,7 @@ do {									\
 		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
 	}								\
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 /*
@@ -328,6 +329,7 @@ extern unsigned long task_size_32bit(void);
 extern unsigned long task_size_64bit(int full_addr_space);
 extern unsigned long get_mmap_base(int is_legacy);
 extern bool mmap_address_hint_valid(unsigned long addr, unsigned long len);
+extern unsigned long get_sigframe_size(void);
 
 #ifdef CONFIG_X86_32
 
@@ -349,6 +351,7 @@ do {									\
 	if (vdso64_enabled)						\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
 			    (unsigned long __force)current->mm->context.vdso); \
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 /* As a historical oddity, the x32 and x86_64 vDSOs are controlled together. */
@@ -357,6 +360,7 @@ do {									\
 	if (vdso64_enabled)						\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
 			    (unsigned long __force)current->mm->context.vdso); \
+	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
 } while (0)
 
 #define AT_SYSINFO		32
diff --git a/arch/x86/include/uapi/asm/auxvec.h b/arch/x86/include/uapi/asm/auxvec.h
index 580e3c567046..6beb55bbefa4 100644
--- a/arch/x86/include/uapi/asm/auxvec.h
+++ b/arch/x86/include/uapi/asm/auxvec.h
@@ -12,9 +12,9 @@
 
 /* entries in ARCH_DLINFO: */
 #if defined(CONFIG_IA32_EMULATION) || !defined(CONFIG_X86_64)
-# define AT_VECTOR_SIZE_ARCH 2
+# define AT_VECTOR_SIZE_ARCH 3
 #else /* else it's non-compat x86-64 */
-# define AT_VECTOR_SIZE_ARCH 1
+# define AT_VECTOR_SIZE_ARCH 2
 #endif
 
 #endif /* _ASM_X86_AUXVEC_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 800243afd1ef..0d24f64d0145 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -716,6 +716,11 @@ void __init init_sigframe_size(void)
 	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
 }
 
+unsigned long get_sigframe_size(void)
+{
+	return max_frame_size;
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&
-- 
2.17.1

Re: [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Borislav Petkov]

On Sat, Feb 27, 2021 at 08:59:08AM -0800, Chang S. Bae wrote:
> Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
> use by all architectures with sigaltstack(2). Over time, the hardware state
> size grew, but these constants did not evolve. Today, literal use of these
> constants on several architectures may result in signal stack overflow, and
> thus user data corruption.
> 
> A few years ago, the ARM team addressed this issue by establishing
> getauxval(AT_MINSIGSTKSZ). This enables the kernel to supply at runtime
> value that is an appropriate replacement on the current and future
> hardware.
> 
> Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
> added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
> size to userspace via auxv").
> 
> Also, include a documentation to describe x86-specific auxiliary vectors.
> 
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")

Right, so this has a Fixes: tag and points to bugzilla entry which talks
about signal stack corruption with AVX-512F.

But if this is going to be backported to stable, then the patch(es)
should be minimal and not contain documentation. And if so, one will
need all three to be backported, which means, a cc:stable should contain
a comment explaining that.

Or am I misreading and they should not need to be backported to stable
because some <non-obvious reason>?

Also, I'm not sure backporting a patch to stable which changes ABI is
ok. It probably is but I don't know.

So what's the deal here?

> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Cc: H.J. Lu <hjl.tools@gmail.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: x86@kernel.org
> Cc: libc-alpha@sourceware.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-api@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
> ---
> Changes from v5:
> * Added a documentation.
> ---
>  Documentation/x86/elf_auxvec.rst   | 56 ++++++++++++++++++++++++++++++
>  arch/x86/include/asm/elf.h         |  4 +++
>  arch/x86/include/uapi/asm/auxvec.h |  4 +--
>  arch/x86/kernel/signal.c           |  5 +++
>  4 files changed, 67 insertions(+), 2 deletions(-)

You also need:

diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
index 4693e192b447..d58614d5cde6 100644
--- a/Documentation/x86/index.rst
+++ b/Documentation/x86/index.rst
@@ -35,3 +35,4 @@ x86-specific Documentation
    sva
    sgx
    features
+   elf_auxvec

to add this to the TOC.

>  create mode 100644 Documentation/x86/elf_auxvec.rst
> 
> diff --git a/Documentation/x86/elf_auxvec.rst b/Documentation/x86/elf_auxvec.rst
> new file mode 100644
> index 000000000000..751c552c4048
> --- /dev/null
> +++ b/Documentation/x86/elf_auxvec.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==================================
> +x86-specific ELF Auxiliary Vectors
> +==================================
> +
> +This document describes the semantics of the x86 auxiliary vectors.
> +
> +1. Introduction
> +---------------
> +
> +ELF Auxiliary vectors enable the kernel to efficiently provide
> +configuration specific parameters to userspace. In this example, a program
> +allocates an alternate stack based on the kernel-provided size.
> +
> +   #include <sys/auxv.h>
> +   #include <elf.h>
> +
> +   #ifndef AT_MINSIGSTKSZ
> +   #define AT_MINSIGSTKSZ	51
> +   #endif
> +
> +   stack_t ss;
> +   int err;
> +
> +   ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
> +   ss.ss_sp = malloc(ss.ss_size);
> +   ...
> +
> +   err = sigaltstack(&ss, NULL);
> +   ...

That source code needs some special markup to look like source code -
currently, the result looks bad.

> +
> +
> +2. The exposed auxiliary vectors
> +---------------------------------
> +
> +AT_SYSINFO
> +    The entry point to the system call function the virtual Dynamic Shared
> +    Object (vDSO), not exported on 64-bit.

I can't parse that sentence.

> +
> +AT_SYSINFO_EHDR
> +    The start address of the page containing vDSO.
						^
						the


> +
> +AT_MINSIGSTKSZ
> +    The minimum stack size required to deliver a signal. It is a calculated
> +    sigframe size based on the largest possible user context. When programs
> +    use sigaltstack() to provide alternate signal stack, that stack must be
> +    at least the size to function properly on this hardware. Note that this
> +    is a minimum of the kernel to correctly get to the signal handler.

I get what this is trying to say but it reads weird. Simplify pls.

> +    Additional space must be added to handle objects pushed onto the stack
> +    by the signal handlers, as well as for nested signal delivery.
> +
> +    The purpose of this parameter is to accommodate the different stack
> +    sizes required by different hardware configuration. E.g., the x86
> +    system supporting the Advanced Vector Extension needs at least 8KB more
> +    than the one without it.

That could be simplified too.

> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 66bdfe838d61..cd10795c178e 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -312,6 +312,7 @@ do {									\
>  		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
>  		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
>  	}								\
> +	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\

Check vertical alignment of the '\'

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Bae, Chang Seok]

On Mar 5, 2021, at 02:43, Borislav Petkov <bp@suse.de> wrote:
> On Sat, Feb 27, 2021 at 08:59:08AM -0800, Chang S. Bae wrote:
>> Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
>> use by all architectures with sigaltstack(2). Over time, the hardware state
>> size grew, but these constants did not evolve. Today, literal use of these
>> constants on several architectures may result in signal stack overflow, and
>> thus user data corruption.
>> 
>> A few years ago, the ARM team addressed this issue by establishing
>> getauxval(AT_MINSIGSTKSZ). This enables the kernel to supply at runtime
>> value that is an appropriate replacement on the current and future
>> hardware.
>> 
>> Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
>> added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
>> size to userspace via auxv").
>> 
>> Also, include a documentation to describe x86-specific auxiliary vectors.
>> 
>> Reported-by: Florian Weimer <fweimer@redhat.com>
>> Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
> 
> Right, so this has a Fixes: tag and points to bugzilla entry which talks
> about signal stack corruption with AVX-512F.
> 
> But if this is going to be backported to stable, then the patch(es)
> should be minimal and not contain documentation. And if so, one will
> need all three to be backported, which means, a cc:stable should contain
> a comment explaining that.
> 
> Or am I misreading and they should not need to be backported to stable
> because some <non-obvious reason>?
> 
> Also, I'm not sure backporting a patch to stable which changes ABI is
> ok. It probably is but I don't know.
> 
> So what's the deal here?

Yeah, right. While this attempts to fix the issue, it involves the ABI change.
Len and I think PATCH5 [1] is rather a backport candidate as it gives a more
reasonable behavior.

At least, I can make a new patch for this documentation if you think it is the
right way.

> You also need:
> 
> diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
> index 4693e192b447..d58614d5cde6 100644
> --- a/Documentation/x86/index.rst
> +++ b/Documentation/x86/index.rst
> @@ -35,3 +35,4 @@ x86-specific Documentation
>    sva
>    sgx
>    features
> +   elf_auxvec
> 
> to add this to the TOC.

Ah, will do that.

>> +   #include <sys/auxv.h>
>> +   #include <elf.h>
>> +
>> +   #ifndef AT_MINSIGSTKSZ
>> +   #define AT_MINSIGSTKSZ	51
>> +   #endif
>> +
>> +   stack_t ss;
>> +   int err;
>> +
>> +   ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
>> +   ss.ss_sp = malloc(ss.ss_size);
>> +   ...
>> +
>> +   err = sigaltstack(&ss, NULL);
>> +   ...
> 
> That source code needs some special markup to look like source code -
> currently, the result looks bad.

How about this code:

#include <sys/auxv.h>
#include <elf.h>
#include <signal.h>
#include <stdlib.h>
#include <assert.h>
#include <err.h>

#ifndef AT_MINSIGSTKSZ
#define AT_MINSIGSTKSZ	51
#endif

stack_t ss;

ss.ss_sp = malloc(ss.ss_size);
assert(ss.ss_sp);

ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
ss.ss_flags = 0;

if (sigaltstack(&ss, NULL))
	err(1, "sigaltstack");


>> +2. The exposed auxiliary vectors
>> +---------------------------------
>> +
>> +AT_SYSINFO
>> +    The entry point to the system call function the virtual Dynamic Shared
>> +    Object (vDSO), not exported on 64-bit.
> 
> I can't parse that sentence.
> 
>> +
>> +AT_SYSINFO_EHDR
>> +    The start address of the page containing vDSO.
> 						^
> 						the
>> +
>> +AT_MINSIGSTKSZ
>> +    The minimum stack size required to deliver a signal. It is a calculated
>> +    sigframe size based on the largest possible user context. When programs
>> +    use sigaltstack() to provide alternate signal stack, that stack must be
>> +    at least the size to function properly on this hardware. Note that this
>> +    is a minimum of the kernel to correctly get to the signal handler.
> 
> I get what this is trying to say but it reads weird. Simplify pls.
> 
>> +    Additional space must be added to handle objects pushed onto the stack
>> +    by the signal handlers, as well as for nested signal delivery.
>> +
>> +    The purpose of this parameter is to accommodate the different stack
>> +    sizes required by different hardware configuration. E.g., the x86
>> +    system supporting the Advanced Vector Extension needs at least 8KB more
>> +    than the one without it.
> 
> That could be simplified too.

Rewrote like this:

AT_SYSINFO is used for locating the vsyscall entry point.  It is not exported
on 64-bit mode.

AT_SYSINFO_EHDR is the start address of the page containing the vDSO.

AT_MINSIGSTKSZ denotes the minimum stack size required by the kernel to
deliver a signal to user-space.  AT_MINSIGSTKSZ comprehends the space consumed
by the kernel to accommodate the user context for the current hardware
configuration.  It does not comprehend subsequent user-space stack
consumption, which must be added by the user.  (e.g. Above, user-space adds
SIGSTKSZ to AT_MINSIGSTKSZ.)

>> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
>> index 66bdfe838d61..cd10795c178e 100644
>> --- a/arch/x86/include/asm/elf.h
>> +++ b/arch/x86/include/asm/elf.h
>> @@ -312,6 +312,7 @@ do {									\
>> 		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
>> 		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
>> 	}								\
>> +	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
> 
> Check vertical alignment of the '\'

Sorry, I will make sure this next time.

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20210227165911.32757-6-chang.seok.bae@intel.com/

Re: [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Borislav Petkov]

On Wed, Mar 10, 2021 at 04:34:40PM +0000, Bae, Chang Seok wrote:
> Yeah, right. While this attempts to fix the issue, it involves the ABI change.
> Len and I think PATCH5 [1] is rather a backport candidate as it gives a more
> reasonable behavior.

Yap, patch 5 is more conservative, sure.

I guess I can take it out of the set and send it Linuswards now.

> > That source code needs some special markup to look like source code -
> > currently, the result looks bad.
> 
> How about this code:

No, build the docs by doing

make htmldocs

and look at the result.

Btw, you should always look at the result before sending it out.

> Rewrote like this:
> 
> AT_SYSINFO is used for locating the vsyscall entry point.  It is not exported
> on 64-bit mode.
> 
> AT_SYSINFO_EHDR is the start address of the page containing the vDSO.
> 
> AT_MINSIGSTKSZ denotes the minimum stack size required by the kernel to
> deliver a signal to user-space.  AT_MINSIGSTKSZ comprehends the space consumed
> by the kernel to accommodate the user context for the current hardware
> configuration.  It does not comprehend subsequent user-space stack
> consumption, which must be added by the user.  (e.g. Above, user-space adds
> SIGSTKSZ to AT_MINSIGSTKSZ.)

Better.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Bae, Chang Seok]

On Mar 10, 2021, at 09:43, Borislav Petkov <bp@suse.de> wrote:
> 
> No, build the docs by doing
> 
> make htmldocs
> 
> and look at the result.
> 
> Btw, you should always look at the result before sending it out.

Ah, now I get that. Sorry for my ignorance. I will fix and make sure not
missing this check.

Thanks,
Chang