On Mar 5, 2021, at 02:43, Borislav Petkov [off-list ref] wrote:

On Sat, Feb 27, 2021 at 08:59:08AM -0800, Chang S. Bae wrote:

quoted

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

Ah, will do that.

quoted

+   #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");

quoted

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

quoted

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

						^
						the

quoted

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

quoted

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

quoted

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/ (local)