x86/fault: #PF improvements, mostly related to USER bit

One of Linus' favorite hobbies seems to be looking at OOPSes and decoding the error code in his head. This is not one of my favorite hobbies :)

Teach the page fault OOPS hander to decode the error code. If it's a !USER fault from user mode, print an explicit note to that effect and print out the addresses of various tables that might cause such an error.

With this patch applied, if I intentionally point the LDT at 0x0 and run the x86 selftests, I get:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000

HW error: normal kernel read fault
This was a system access from user code
IDT: 0xfffffe0000000000 (limit=0xfff) GDT: 0xfffffe0000001000 (limit=0x7f)
LDTR: 0x50 -- base=0x0 limit=0xfff7
TR: 0x40 -- base=0xfffffe0000003000 limit=0x206f
PGD 800000000456e067 P4D 800000000456e067 PUD 4623067 PMD 0 SMP PTI

CPU: 0 PID: 153 Comm: ldt_gdt_64 Not tainted 4.19.0+ #1317
Hardware name: ...
RIP: 0033:0x401454

HW error: normal kernel read fault

Quoted 20 lines · Andy Lutomirski

One of Linus' favorite hobbies seems to be looking at OOPSes and decoding the error code in his head. This is not one of my favorite hobbies :)

Teach the page fault OOPS hander to decode the error code. If it's a !USER fault from user mode, print an explicit note to that effect and print out the addresses of various tables that might cause such an error.

With this patch applied, if I intentionally point the LDT at 0x0 and run the x86 selftests, I get:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000

HW error: normal kernel read fault
This was a system access from user code
IDT: 0xfffffe0000000000 (limit=0xfff) GDT: 0xfffffe0000001000 (limit=0x7f)
LDTR: 0x50 -- base=0x0 limit=0xfff7
TR: 0x40 -- base=0xfffffe0000003000 limit=0x206f
PGD 800000000456e067 P4D 800000000456e067 PUD 4623067 PMD 0 SMP PTI

Quoted 3 lines

CPU: 0 PID: 153 Comm: ldt_gdt_64 Not tainted 4.19.0+ #1317
Hardware name: ...
RIP: 0033:0x401454

I've applied your series, with one small edit, the following message:

Quoted 1 line

HW error: normal kernel read fault

will IMHO confuse the heck out of users, thinking that their hardware is broken...

Yes, the message is accurate, in MM pagefault language it's indeed the HW error code, but it's a language very few people speak.

So I edited it over to say '#PF error code'. I also applied a few other minor cleanups - see the changelog below.

Let me know if you have any objections.

Thanks,

Ingo

===============>
From a2aa52ab16efbee40ad118ebac4a5e438f5b43ee Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Thu, 22 Nov 2018 09:34:03 +0100
Subject: [PATCH] x86/fault: Clean up the page fault oops decoder a bit

- Make the oops messages a bit less scary (don't mention 'HW errors')

- Turn 'PROT USER' (which is visually easily confused with PROT_USER) into individual bit descriptors: "[PROT] [USER]".
This also makes "[normal kernel read fault]" more apparent.

- De-abbreviate variables to make the code easier to read

- Use vertical alignment where appropriate.

- Add comment about string size limits and the helper function.

- Remove unnecessary line breaks.

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Yu-cheng Yu <[email protected]>

Cc [email protected]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mm/fault.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

arch/x86/mm/fault.c +23 -15

index f5efbdba2b6d..2ff25ad33233 100644
@@ -603,10 +603,13 @@ static void show_ldttss(const struct desc_ptr *gdt, const char *name, u16 index)
 		 name, index, addr, (desc.limit0 | (desc.limit1 << 16)));
 }
 
-static void errstr(unsigned long ec, char *buf, unsigned long mask,
-		   const char *txt)
+/*
+ * This helper function transforms the #PF error_code bits into
+ * "[PROT] [USER]" type of descriptive, almost human-readable error strings:
+ */
+static void err_str_append(unsigned long error_code, char *buf, unsigned long mask, const char *txt)
 {
-	if (ec & mask) {
+	if (error_code & mask) {
 		if (buf[0])
 			strcat(buf, " ");
 		strcat(buf, txt);

@@ -614,10 +617,9 @@ static void errstr(unsigned long ec, char *buf, unsigned long mask,
 }
 
 static void
-show_fault_oops(struct pt_regs *regs, unsigned long error_code,
-		unsigned long address)
+show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long address)
 {
-	char errtxt[64];
+	char err_txt[64];
 
 	if (!oops_may_print())
 		return;

@@ -646,15 +648,21 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code,
 		 address < PAGE_SIZE ? "NULL pointer dereference" : "paging request",
 		 (void *)address);
 
-	errtxt[0] = 0;
-	errstr(error_code, errtxt, X86_PF_PROT, "PROT");
-	errstr(error_code, errtxt, X86_PF_WRITE, "WRITE");
-	errstr(error_code, errtxt, X86_PF_USER, "USER");
-	errstr(error_code, errtxt, X86_PF_RSVD, "RSVD");
-	errstr(error_code, errtxt, X86_PF_INSTR, "INSTR");
-	errstr(error_code, errtxt, X86_PF_PK, "PK");
-	pr_alert("HW error: %s\n", error_code ? errtxt :
-		 "normal kernel read fault");
+	err_txt[0] = 0;
+
+	/*
+	 * Note: length of these appended strings including the separation space and the
+	 * zero delimiter must fit into err_txt[].
+	 */
+	err_str_append(error_code, err_txt, X86_PF_PROT,  "[PROT]" );
+	err_str_append(error_code, err_txt, X86_PF_WRITE, "[WRITE]");
+	err_str_append(error_code, err_txt, X86_PF_USER,  "[USER]" );
+	err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
+	err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
+	err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
+
+	pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");
+
 	if (!(error_code & X86_PF_USER) && user_mode(regs)) {
 		struct desc_ptr idt, gdt;
 		u16 ldtr, tr;

Quoted 31 lines · Ingo Molnar

Quoted 20 lines · Andy Lutomirski

One of Linus' favorite hobbies seems to be looking at OOPSes and decoding the error code in his head. This is not one of my favorite hobbies :)

Teach the page fault OOPS hander to decode the error code. If it's a !USER fault from user mode, print an explicit note to that effect and print out the addresses of various tables that might cause such an error.

With this patch applied, if I intentionally point the LDT at 0x0 and run the x86 selftests, I get:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000

HW error: normal kernel read fault
This was a system access from user code
IDT: 0xfffffe0000000000 (limit=0xfff) GDT: 0xfffffe0000001000 (limit=0x7f)
LDTR: 0x50 -- base=0x0 limit=0xfff7
TR: 0x40 -- base=0xfffffe0000003000 limit=0x206f
PGD 800000000456e067 P4D 800000000456e067 PUD 4623067 PMD 0 SMP PTI

Quoted 3 lines

CPU: 0 PID: 153 Comm: ldt_gdt_64 Not tainted 4.19.0+ #1317
Hardware name: ...
RIP: 0033:0x401454

I've applied your series, with one small edit, the following message:

Quoted 1 line

HW error: normal kernel read fault

Quoted 8 lines

will IMHO confuse the heck out of users, thinking that their hardware is broken...

Yes, the message is accurate, in MM pagefault language it's indeed the HW error code, but it's a language very few people speak.

So I edited it over to say '#PF error code'. I also applied a few other minor cleanups - see the changelog below.

I responded to the original thread a hair too late...

What about something like this instead of manually handling the case where error_code==0 so that we get e.g. "[KERNEL] [READ]" instead of "normal kernel read fault"? Getting "[PROT] [KERNEL] [READ]" seems useful.

IMO "[normal kernel read fault]" followed by "This was a system access from user code" is still confusing.

---
8b29ee4351d5c625aa9ca2765f8da5e Mon Sep 17 00:00:00 2001
From: Sean Christopherson <[email protected]>
Date: Tue, 27 Nov 2018 07:09:57 -0800
Subject: [PATCH] x86/fault: Print "KERNEL" and "READ" for #PF error codes

...and explicitly state that it's a "code" that's being printed.

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Yu-cheng Yu <[email protected]>

Cc [email protected]

Cc Ingo Molnar <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/mm/fault.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/x86/mm/fault.c +0 -0

index 2ff25ad33233..510e263c256b 100644

Quoted 12 lines · Sean Christopherson

arch/x86/mm/fault.c +2 -2

index 2ff25ad33233..510e263c256b 100644
@@ -660,8 +660,10 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
        err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
        err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
        err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
-
-       pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");
+       err_str_append(~error_code, err_txt, X86_PF_USER, "[KERNEL]");
+       err_str_append(~error_code, err_txt, X86_PF_WRITE | X86_PF_INSTR,

arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad · line 667

Quoted 3 lines

arch/x86/mm/fault.c +2 -0

+                                                         "[READ]");
+       pr_alert("#PF error code: %s\n", err_txt);
 

Seems generally nice, but I would suggest making the bit-not-set name be another parameter to err_str_append(). I'm also slightly uneasy about making "KERNEL" look like a bit, but I guess it doesn't bother me too much.

Want to send a real patch?

Quoted 14 lines · Andy Lutomirski

Quoted 12 lines · Sean Christopherson

arch/x86/mm/fault.c +2 -2

index 2ff25ad33233..510e263c256b 100644
@@ -660,8 +660,10 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
        err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
        err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
        err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
-
-       pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");
+       err_str_append(~error_code, err_txt, X86_PF_USER, "[KERNEL]");
+       err_str_append(~error_code, err_txt, X86_PF_WRITE | X86_PF_INSTR,

Quoted 8 lines

arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad · line 667

Quoted 3 lines

arch/x86/mm/fault.c +2 -0

+                                                         "[READ]");
+       pr_alert("#PF error code: %s\n", err_txt);
 

Seems generally nice, but I would suggest making the bit-not-set name be another parameter to err_str_append(). I'm also slightly uneasy about making "KERNEL" look like a bit, but I guess it doesn't bother me too much.

What about "SUPERVISOR" instead of "KERNEL"? It'd be consistent with the SDM and hopefully less likely to be misconstrued as something else.

Quoted 25 lines · Sean Christopherson

Quoted 22 lines · Andy Lutomirski

Quoted 5 lines · Sean Christopherson

arch/x86/mm/fault.c +0 -0

index 2ff25ad33233..510e263c256b 100644

arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad · line 667

Quoted 10 lines

arch/x86/mm/fault.c +4 -2

@@ -660,8 +660,10 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
        err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
        err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
        err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
-
-       pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");
+       err_str_append(~error_code, err_txt, X86_PF_USER, "[KERNEL]");
+       err_str_append(~error_code, err_txt, X86_PF_WRITE | X86_PF_INSTR,
+                                                         "[READ]");
+       pr_alert("#PF error code: %s\n", err_txt);
 

Seems generally nice, but I would suggest making the bit-not-set name be another parameter to err_str_append(). I'm also slightly uneasy about making "KERNEL" look like a bit, but I guess it doesn't bother me too much.

Quoted 2 lines

What about "SUPERVISOR" instead of "KERNEL"? It'd be consistent with the SDM and hopefully less likely to be misconstrued as something else.

Or even just [!USER], perhaps.

Quoted 30 lines · Andy Lutomirski

Quoted 27 lines · Sean Christopherson

Quoted 14 lines · Andy Lutomirski

Quoted 12 lines · Sean Christopherson

arch/x86/mm/fault.c +2 -2

index 2ff25ad33233..510e263c256b 100644
@@ -660,8 +660,10 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
        err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
        err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
        err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
-
-       pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");
+       err_str_append(~error_code, err_txt, X86_PF_USER, "[KERNEL]");
+       err_str_append(~error_code, err_txt, X86_PF_WRITE | X86_PF_INSTR,

Quoted 8 lines

arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad · line 667

Quoted 3 lines

arch/x86/mm/fault.c +2 -0

+                                                         "[READ]");
+       pr_alert("#PF error code: %s\n", err_txt);
 

Seems generally nice, but I would suggest making the bit-not-set name be another parameter to err_str_append(). I'm also slightly uneasy about making "KERNEL" look like a bit, but I guess it doesn't bother me too much.

What about "SUPERVISOR" instead of "KERNEL"? It'd be consistent with the SDM and hopefully less likely to be misconstrued as something else.

Quoted 1 line

Or even just [!USER], perhaps.

I thought about that too, but the pedant in me didn't like the inconsistency of doing "READ" instead of "[!WRITE] [!INSTR]", and IMO "READ" is a lot more readable (no pun intended). I also like having completely different text, makes it harder to miss a single "!" and go down the wrong path.

Quoted 13 lines · Sean Christopherson

 arch/x86/mm/fault.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/x86/mm/fault.c +0 -2

index 2ff25ad33233..510e263c256b 100644
@@ -660,8 +660,10 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
        err_str_append(error_code, err_txt, X86_PF_RSVD,  "[RSVD]" );
        err_str_append(error_code, err_txt, X86_PF_INSTR, "[INSTR]");
        err_str_append(error_code, err_txt, X86_PF_PK,    "[PK]"   );
-
-       pr_alert("#PF error: %s\n", error_code ? err_txt : "[normal kernel read fault]");

arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad · line 667

Quoted 5 lines

arch/x86/mm/fault.c +4 -0

+       err_str_append(~error_code, err_txt, X86_PF_USER, "[KERNEL]");
+       err_str_append(~error_code, err_txt, X86_PF_WRITE | X86_PF_INSTR,
+                                                         "[READ]");
+       pr_alert("#PF error code: %s\n", err_txt);
 

Seems generally nice, but I would suggest making the bit-not-set name be another parameter to err_str_append().

This series is a whole bunch of page fault cleanups, plus a couple of OOPS diagnostic improvements. The overall goals are to clean up handling of the faulting CPL, the USER bit in the error_code, and the log messages generated by #PF OOPSes.

This series can also be seen as CET preparation. CET introduces the WRUSS instruction, which is the very first way for CPL 0 code to cause a #PF fault with the USER bit set. Let's get the page fault code into shape before we start using WRUSS :)

Changes from v1:
- Rebase on top of tip:x86/mm, now that a bunch of v1 was applied. The only material changes are that 'x86/fault: Check user_mode(regs) when validating a stack extension' is gone because the code it fixed has been deleted and that 'x86/fault:
Remove sw_error_code' lost the hunk that changed the same code.

Andy Lutomirski (5):
x86/fault: Remove sw_error_code
x86/fault: Don't try to recover from an implicit supervisor access x86/oops: Show the correct CS value in show_regs() x86/fault: Decode page fault OOPSes better x86/vsyscall/64: Use X86_PF constants in the simulated #PF error code

 arch/x86/entry/vsyscall/vsyscall_64.c |   2 +-
 arch/x86/kernel/process_64.c          |   5 +-
 arch/x86/mm/fault.c                   | 144 +++++++++++++++++++-------
 3 files changed, 108 insertions(+), 43 deletions(-)