Skip to contentSkip to conversation

A fix and a cleanup to the x86 sigreturn selftests

PATCH New Andy Lutomirski 2 participants 5 messages Jun 27, 2018

Subsystems (2)
x86 Kselftest
lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

cover A fix and a cleanup to the x86 sigreturn selftests

The sigreturn_64 selftest has always failed on AMD CPUs. Fix it and clean it up a bit.

Andy Lutomirski (2):
selftests/x86/sigreturn/64: Fix spurious failures on AMD CPUs selftests/x86/sigreturn: Minor cleanups

 tools/testing/selftests/x86/sigreturn.c | 59 +++++++++++++++----------
 1 file changed, 36 insertions(+), 23 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/2 selftests/x86/sigreturn/64: Fix spurious failures on AMD CPUs

When I wrote the sigreturn test, I didn't realize that AMD's busted IRET behavior was different from Intel's busted IRET behavior. On AMD CPUs, the CPU leaks the high 32 bits of the kernel stack pointer to certain userspace contexts. Gee, thanks. There's very little the kernel can do about it. Modify the test so it passes.

Signed-off-by Andy Lutomirski <[email protected]>
---
 tools/testing/selftests/x86/sigreturn.c | 46 ++++++++++++++++---------
 1 file changed, 29 insertions(+), 17 deletions(-)
tools/testing/selftests/x86/sigreturn.c +29 -17
index 246145b84a12..2559e2c01793 100644
@@ -612,19 +612,38 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 		greg_t req = requested_regs[i], res = resulting_regs[i];
 		if (i == REG_TRAPNO || i == REG_IP)
 			continue;	/* don't care */
-		if (i == REG_SP) {
-			printf("\tSP: %llx -> %llx\n", (unsigned long long)req,
-			       (unsigned long long)res);
 
+		if (i == REG_SP) {
 			/*
-			 * In many circumstances, the high 32 bits of rsp
-			 * are zeroed.  For example, we could be a real
-			 * 32-bit program, or we could hit any of a number
-			 * of poorly-documented IRET or segmented ESP
-			 * oddities.  If this happens, it's okay.
+			 * If we were using a 16-bit stack segment, then
+			 * the kernel is a bit stuck: IRET only restores
+			 * the low 16 bits of ESP/RSP if SS is 16-bit.
+			 * The kernel uses a hack to restore bits 31:16,
+			 * but that hack doesn't help with bits 63:32.
+			 * On Intel CPUs, bits 63:32 end up zeroed, and, on
+			 * AMD CPUs, they leak the high bits of the kernel
+			 * espfix64 stack pointer.  There's very little that
+			 * the kernel can do about it.
+			 *
+			 * Similarly, if we are returning to a 32-bit context,
+			 * the CPU will often lose the high 32 bits of RSP.
 			 */
-			if (res == (req & 0xFFFFFFFF))
-				continue;  /* OK; not expected to work */
+
+			if (res == req)
+				continue;
+
+			if (cs_bits != 64 && ((res ^ req) & 0xFFFFFFFF) == 0) {
+				printf("[NOTE]\tSP: %llx -> %llx\n",
+				       (unsigned long long)req,
+				       (unsigned long long)res);
+				continue;
+			}
+
+			printf("[FAIL]\tSP mismatch: requested 0x%llx; got 0x%llx\n",
+			       (unsigned long long)requested_regs[i],
+			       (unsigned long long)resulting_regs[i]);
+			nerrs++;
+			continue;
 		}
 
 		bool ignore_reg = false;

@@ -663,13 +682,6 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 		}
 
 		if (requested_regs[i] != resulting_regs[i] && !ignore_reg) {
-			/*
-			 * SP is particularly interesting here.  The
-			 * usual cause of failures is that we hit the
-			 * nasty IRET case of returning to a 16-bit SS,
-			 * in which case bits 16:31 of the *kernel*
-			 * stack pointer persist in ESP.
-			 */
 			printf("[FAIL]\tReg %d mismatch: requested 0x%llx; got 0x%llx\n",
 			       i, (unsigned long long)requested_regs[i],
 			       (unsigned long long)resulting_regs[i]);
Signature · 1 line
2.17.1
merge bot
#2 lore raw

[tip:x86/urgent] selftests/x86/sigreturn/64: Fix spurious failures on AMD CPUs

selftests/x86/sigreturn/64: Fix spurious failures on AMD CPUs
Commit-ID:  ec348020566009d3da9b99f07c05814d13969c78
Gitweb:     https://git.kernel.org/tip/ec348020566009d3da9b99f07c05814d13969c78
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 26 Jun 2018 22:17:17 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 27 Jun 2018 09:36:56 +0200

selftests/x86/sigreturn/64: Fix spurious failures on AMD CPUs

When I wrote the sigreturn test, I didn't realize that AMD's busted IRET behavior was different from Intel's busted IRET behavior:

On AMD CPUs, the CPU leaks the high 32 bits of the kernel stack pointer to certain userspace contexts. Gee, thanks. There's very little the kernel can do about it. Modify the test so it passes.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/86e7fd3564497f657de30a36da4505799eebef01.1530076529.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 tools/testing/selftests/x86/sigreturn.c | 46 +++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 17 deletions(-)
tools/testing/selftests/x86/sigreturn.c +29 -17
index 246145b84a12..2559e2c01793 100644
@@ -612,19 +612,38 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 		greg_t req = requested_regs[i], res = resulting_regs[i];
 		if (i == REG_TRAPNO || i == REG_IP)
 			continue;	/* don't care */
-		if (i == REG_SP) {
-			printf("\tSP: %llx -> %llx\n", (unsigned long long)req,
-			       (unsigned long long)res);
 
+		if (i == REG_SP) {
 			/*
-			 * In many circumstances, the high 32 bits of rsp
-			 * are zeroed.  For example, we could be a real
-			 * 32-bit program, or we could hit any of a number
-			 * of poorly-documented IRET or segmented ESP
-			 * oddities.  If this happens, it's okay.
+			 * If we were using a 16-bit stack segment, then
+			 * the kernel is a bit stuck: IRET only restores
+			 * the low 16 bits of ESP/RSP if SS is 16-bit.
+			 * The kernel uses a hack to restore bits 31:16,
+			 * but that hack doesn't help with bits 63:32.
+			 * On Intel CPUs, bits 63:32 end up zeroed, and, on
+			 * AMD CPUs, they leak the high bits of the kernel
+			 * espfix64 stack pointer.  There's very little that
+			 * the kernel can do about it.
+			 *
+			 * Similarly, if we are returning to a 32-bit context,
+			 * the CPU will often lose the high 32 bits of RSP.
 			 */
-			if (res == (req & 0xFFFFFFFF))
-				continue;  /* OK; not expected to work */
+
+			if (res == req)
+				continue;
+
+			if (cs_bits != 64 && ((res ^ req) & 0xFFFFFFFF) == 0) {
+				printf("[NOTE]\tSP: %llx -> %llx\n",
+				       (unsigned long long)req,
+				       (unsigned long long)res);
+				continue;
+			}
+
+			printf("[FAIL]\tSP mismatch: requested 0x%llx; got 0x%llx\n",
+			       (unsigned long long)requested_regs[i],
+			       (unsigned long long)resulting_regs[i]);
+			nerrs++;
+			continue;
 		}
 
 		bool ignore_reg = false;

@@ -663,13 +682,6 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 		}
 
 		if (requested_regs[i] != resulting_regs[i] && !ignore_reg) {
-			/*
-			 * SP is particularly interesting here.  The
-			 * usual cause of failures is that we hit the
-			 * nasty IRET case of returning to a 16-bit SS,
-			 * in which case bits 16:31 of the *kernel*
-			 * stack pointer persist in ESP.
-			 */
 			printf("[FAIL]\tReg %d mismatch: requested 0x%llx; got 0x%llx\n",
 			       i, (unsigned long long)requested_regs[i],
 			       (unsigned long long)resulting_regs[i]);
author
#3 lore raw

PATCH 2/2 selftests/x86/sigreturn: Minor cleanups

In reply to Andy Lutomirski

We have short names for the requested and resulting register values. Use them instead of spelling out the whole register entry for each case.

Signed-off-by Andy Lutomirski <[email protected]>
---
 tools/testing/selftests/x86/sigreturn.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
tools/testing/selftests/x86/sigreturn.c +7 -6
index 2559e2c01793..4d9dc3f2fd70 100644
@@ -610,6 +610,7 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 	 */
 	for (int i = 0; i < NGREG; i++) {
 		greg_t req = requested_regs[i], res = resulting_regs[i];
+
 		if (i == REG_TRAPNO || i == REG_IP)
 			continue;	/* don't care */
 

@@ -673,18 +674,18 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 #endif
 
 		/* Sanity check on the kernel */
-		if (i == REG_CX && requested_regs[i] != resulting_regs[i]) {
+		if (i == REG_CX && req != res) {
 			printf("[FAIL]\tCX (saved SP) mismatch: requested 0x%llx; got 0x%llx\n",
-			       (unsigned long long)requested_regs[i],
-			       (unsigned long long)resulting_regs[i]);
+			       (unsigned long long)req,
+			       (unsigned long long)res);
 			nerrs++;
 			continue;
 		}
 
-		if (requested_regs[i] != resulting_regs[i] && !ignore_reg) {
+		if (req != res && !ignore_reg) {
 			printf("[FAIL]\tReg %d mismatch: requested 0x%llx; got 0x%llx\n",
-			       i, (unsigned long long)requested_regs[i],
-			       (unsigned long long)resulting_regs[i]);
+			       i, (unsigned long long)req,
+			       (unsigned long long)res);
 			nerrs++;
 		}
 	}
Signature · 1 line
2.17.1
merge bot
#4 lore raw

[tip:x86/urgent] selftests/x86/sigreturn: Do minor cleanups

selftests/x86/sigreturn: Do minor cleanups
Commit-ID:  e8a445dea219c32727016af14f847d2e8f7ebec8
Gitweb:     https://git.kernel.org/tip/e8a445dea219c32727016af14f847d2e8f7ebec8
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 26 Jun 2018 22:17:18 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 27 Jun 2018 09:36:56 +0200

selftests/x86/sigreturn: Do minor cleanups

We have short names for the requested and resulting register values. Use them instead of spelling out the whole register entry for each case.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/bb3bc1f923a2f6fe7912d22a1068fe29d6033d38.1530076529.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 tools/testing/selftests/x86/sigreturn.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
tools/testing/selftests/x86/sigreturn.c +7 -6
index 2559e2c01793..4d9dc3f2fd70 100644
@@ -610,6 +610,7 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 	 */
 	for (int i = 0; i < NGREG; i++) {
 		greg_t req = requested_regs[i], res = resulting_regs[i];
+
 		if (i == REG_TRAPNO || i == REG_IP)
 			continue;	/* don't care */
 

@@ -673,18 +674,18 @@ static int test_valid_sigreturn(int cs_bits, bool use_16bit_ss, int force_ss)
 #endif
 
 		/* Sanity check on the kernel */
-		if (i == REG_CX && requested_regs[i] != resulting_regs[i]) {
+		if (i == REG_CX && req != res) {
 			printf("[FAIL]\tCX (saved SP) mismatch: requested 0x%llx; got 0x%llx\n",
-			       (unsigned long long)requested_regs[i],
-			       (unsigned long long)resulting_regs[i]);
+			       (unsigned long long)req,
+			       (unsigned long long)res);
 			nerrs++;
 			continue;
 		}
 
-		if (requested_regs[i] != resulting_regs[i] && !ignore_reg) {
+		if (req != res && !ignore_reg) {
 			printf("[FAIL]\tReg %d mismatch: requested 0x%llx; got 0x%llx\n",
-			       i, (unsigned long long)requested_regs[i],
-			       (unsigned long long)resulting_regs[i]);
+			       i, (unsigned long long)req,
+			       (unsigned long long)res);
 			nerrs++;
 		}
 	}