x86, signals: add missing signal_compat code for x86 features

Sending this out early so folks can have a look. I haven't let it run through a full set of tests, so buyer beware, but it would have a hard time hurting anything other than the already-broken 32-bit compat signal code.

---

From: Dave Hansen <[email protected]>

The 32-bit siginfo is a different binary format than the 64-bit one. So, when running 32-bit binaries on 64-bit kernels, we have to convert the kernel's 64-bit version to a 32-bit version that userspace can grok.

We've added a few features to siginfo over the past few years and neglected to add them to arch/x86/kernel/signal_compat.c:

1. The si_addr_lsb used in SIGBUS's sent for machine checks
2. The upper/lower bounds for MPX SIGSEGV faults
3. The protection key for pkey faults

I caught this with some protection keys unit tests and realized it affected a few more features.

This was tested only with my protection keys patch that looks for a proper value in si_pkey. I didn't actually test the machine check or MPX code.

---

 b/arch/x86/include/asm/compat.h   |   11 +++++++++++
 b/arch/x86/kernel/signal_compat.c |   13 +++++++++++++
 2 files changed, 24 insertions(+)

diff -puN arch/x86/include/asm/compat.h~add-signal-compat-for-mpx-pkeys arch/x86/include/asm/compat.h
@@ -40,6 +40,7 @@ typedef s32		compat_long_t;
 typedef s64 __attribute__((aligned(4))) compat_s64;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
+typedef u32		compat_u32;
 typedef u64 __attribute__((aligned(4))) compat_u64;
 typedef u32		compat_uptr_t;
 

@@ -181,6 +182,16 @@ typedef struct compat_siginfo {
 		/* SIGILL, SIGFPE, SIGSEGV, SIGBUS */
 		struct {
 			unsigned int _addr;	/* faulting insn/memory ref. */
+			short int _addr_lsb;	/* Valid LSB of the reported address. */
+			union {
+				/* used when si_code=SEGV_BNDERR */
+				struct {
+					compat_uptr_t _lower;
+					compat_uptr_t _upper;
+				} _addr_bnd;
+				/* used when si_code=SEGV_PKUERR */
+				compat_u32 _pkey;
+			};
 		} _sigfault;
 
 		/* SIGPOLL */

diff -puN arch/x86/kernel/signal_compat.c~add-signal-compat-for-mpx-pkeys arch/x86/kernel/signal_compat.c
@@ -32,6 +32,19 @@ int copy_siginfo_to_user32(compat_siginf
 					  &to->_sifields._pad[0]);
 			switch (from->si_code >> 16) {
 			case __SI_FAULT >> 16:
+		                if (from->si_signo == SIGBUS &&
+		                    (from->si_code == BUS_MCEERR_AR ||
+				     from->si_code == BUS_MCEERR_AO))
+					put_user_ex(from->si_addr_lsb, &to->si_addr_lsb);
+
+				if (from->si_signo == SIGSEGV) {
+				       	if (from->si_code == SEGV_BNDERR) {
+						put_user_ex(from->si_lower, &to->si_lower);
+						put_user_ex(from->si_upper, &to->si_upper);
+					}
+				       	if (from->si_code == SEGV_PKUERR)
+						put_user_ex(from->si_pkey, &to->si_pkey);
+				}
 				break;
 			case __SI_SYS >> 16:
 				put_user_ex(from->si_syscall, &to->si_syscall);

_

Hm, while fixing this, could we please also add individual unit tests to tools/testing/selftests/x86/, and also structure the code in a fashion or add a comment or so to make sure future extensions add both a compat handler and a unit test as well?

I.e. perhaps do a (build time) fixed-size check of siginfo structure in the compat code, and break the build if that check has not been updated? Or something like that.

Thanks,

Ingo

The test that found this was one of the tests I'm submitting with protection keys, so that's covered. I also improved my out-of-tree MPX tests to cover this too. I can submit a version of those to be kept in-tree.

Tony / Borislav, do we have tests for the machine check code that could have caught this?

A size check of all the individual parts of the structure would help. But it won't be good enough to catch everything. Protection keys, for example, piggybacked on the space that MPX already carved out. So a size check would not have caught it.

I'll probably go the heavy commenting route, along with some size checks to _help_.

If I had built one of my recovery test programs as a 32-byte binary instead of native 64-bit I might have noticed (I only print the lsb field ... which would have been garbage on the stack, maybe I'd have spotted a silly value).

-Tony