[PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Tony Luck]

From: Zhiquan Li <zhiquan1.li@intel.com>

5.17.0 kernel will crash when we inject MCE by run "einj_mem_uc copyin"
in ras-tools with CONFIG_CC_HAS_ASM_GOTO_OUTPUT != y kernel config.
mce: [Hardware Error]: Machine check events logged
mce: [Hardware Error]: CPU 120: Machine Check Exception: f Bank 1: bd80000000100134
mce: [Hardware Error]: RIP 10: {fault_in_readable+0x9f/0xd0}
mce: [Hardware Error]: TSC 63d3fa6181b69 ADDR f921f31400 MISC 86 PPIN 11a090eb80bf0c9c
mce: [Hardware Error]: PROCESSOR 0:606a6 TIME 1647365323 SOCKET 1 APIC 8d microcode d0002e0
mce: [Hardware Error]: Run the above through 'mcelog --ascii'
mce: [Hardware Error]: Machine check: Data load in unrecoverable area of kernel
Kernel panic - not syncing: Fatal local machine check

In commit 99641e094d6c ("x86/uaccess: Remove .fixup usage"), the
exception type of get_user was changed from EX_TYPE_UACCESS to
EX_TYPE_EFAULT_REG. In case of MCE/SRAR when kernel copy data from user,
the MCE handler identities the exception type with EX_TYPE_UACCESS to
MCE_IN_KERNEL_RECOV. While the new type EX_TYPE_EFAULT_REG will lose
lose the opportunity to rescue the system.

To fix it we have to restore get_user exception type to EX_TYPE_UACCESS,
but we do not want to miss the operation that make reg = -EFAULT in
function ex_handler_imm_reg(), so we so add a new flag EX_FLAG_SET_REG
to identify this case, it will not break anything.

Fixes: 99641e094d6c ("x86/uaccess: Remove .fixup usage")
Cc: <stable@vger.kernel.org> # v5.17+
Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
Co-developed-by: Youquan Song <youquan.song@intel.com>
Signed-off-by: Youquan Song <youquan.song@intel.com>
Tested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

This patch works ... but to test it I had to fake out init/Kconfig so
that it wouldn't set CONFIG_CC_HAS_ASM_GOTO_OUTPUT=y. So it seems that
this is only needed when building with some old compiler version.

With Linus' announcement about C99/C11 as new basis, is this fix
needed? I.e. is it still valid to build the upstream kernel with a
compiler that doesn't grok CONFIG_CC_HAS_ASM_GOTO_OUTPUT?

 arch/x86/include/asm/extable_fixup_types.h |  1 +
 arch/x86/include/asm/uaccess.h             | 15 +++++++++------
 arch/x86/mm/extable.c                      |  8 ++++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index 503622627400..329eeebba2f6 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -30,6 +30,7 @@
 #define EX_FLAG_CLEAR_AX		EX_DATA_FLAG(1)
 #define EX_FLAG_CLEAR_DX		EX_DATA_FLAG(2)
 #define EX_FLAG_CLEAR_AX_DX		EX_DATA_FLAG(3)
+#define EX_FLAG_SET_REG		EX_DATA_FLAG(4)
 
 /* types */
 #define	EX_TYPE_NONE			 0
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index f78e2b3501a1..277f7c87ad81 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -325,11 +325,13 @@ do {									\
 		     "1:	movl %[lowbits],%%eax\n"		\
 		     "2:	movl %[highbits],%%edx\n"		\
 		     "3:\n"						\
-		     _ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_EFAULT_REG |	\
-					   EX_FLAG_CLEAR_AX_DX,		\
+		     _ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_UACCESS |	\
+					   EX_DATA_IMM(-EFAULT) | \
+					   EX_FLAG_CLEAR_AX_DX | EX_FLAG_SET_REG,	\
 					   %[errout])			\
-		     _ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_EFAULT_REG |	\
-					   EX_FLAG_CLEAR_AX_DX,		\
+		     _ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_UACCESS |	\
+					   EX_DATA_IMM(-EFAULT) | \
+					   EX_FLAG_CLEAR_AX_DX | EX_FLAG_SET_REG,	\
 					   %[errout])			\
 		     : [errout] "=r" (retval),				\
 		       [output] "=&A"(x)				\
@@ -372,8 +374,9 @@ do {									\
 	asm volatile("\n"						\
 		     "1:	mov"itype" %[umem],%[output]\n"		\
 		     "2:\n"						\
-		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG | \
-					   EX_FLAG_CLEAR_AX,		\
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_UACCESS |	\
+					   EX_DATA_IMM(-EFAULT) | \
+					   EX_FLAG_CLEAR_AX | EX_FLAG_SET_REG,		\
 					   %[errout])			\
 		     : [errout] "=r" (err),				\
 		       [output] "=a" (x)				\
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index dba2197c05c3..aac599781879 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -183,6 +183,14 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 	case EX_TYPE_FAULT_MCE_SAFE:
 		return ex_handler_fault(e, regs, trapnr);
 	case EX_TYPE_UACCESS:
+		/*
+		 * In the user access case, we might need err reg = -EFAULT, like:
+		 * _ASM_EXTABLE_TYPE_REG(from, to, EX_TYPE_UACCESS | \
+		 *                            EX_DATA_IMM(-EFAULT) | \
+		 *                            EX_FLAG_SET_REG, reg)
+		 */
+		if (e->data & EX_FLAG_SET_REG)
+			*pt_regs_nr(regs, reg) = (long)imm;
 		return ex_handler_uaccess(e, regs, trapnr);
 	case EX_TYPE_COPY:
 		return ex_handler_copy(e, regs, trapnr);
-- 
2.35.1

Re: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Peter Zijlstra]

On Mon, Mar 28, 2022 at 01:17:48PM -0700, Tony Luck wrote:
> From: Zhiquan Li <zhiquan1.li@intel.com>
> 
> 5.17.0 kernel will crash when we inject MCE by run "einj_mem_uc copyin"
> in ras-tools with CONFIG_CC_HAS_ASM_GOTO_OUTPUT != y kernel config.
> mce: [Hardware Error]: Machine check events logged
> mce: [Hardware Error]: CPU 120: Machine Check Exception: f Bank 1: bd80000000100134
> mce: [Hardware Error]: RIP 10: {fault_in_readable+0x9f/0xd0}
> mce: [Hardware Error]: TSC 63d3fa6181b69 ADDR f921f31400 MISC 86 PPIN 11a090eb80bf0c9c
> mce: [Hardware Error]: PROCESSOR 0:606a6 TIME 1647365323 SOCKET 1 APIC 8d microcode d0002e0
> mce: [Hardware Error]: Run the above through 'mcelog --ascii'
> mce: [Hardware Error]: Machine check: Data load in unrecoverable area of kernel
> Kernel panic - not syncing: Fatal local machine check
> 
> In commit 99641e094d6c ("x86/uaccess: Remove .fixup usage"), the
> exception type of get_user was changed from EX_TYPE_UACCESS to
> EX_TYPE_EFAULT_REG. In case of MCE/SRAR when kernel copy data from user,
> the MCE handler identities the exception type with EX_TYPE_UACCESS to
> MCE_IN_KERNEL_RECOV. While the new type EX_TYPE_EFAULT_REG will lose
> lose the opportunity to rescue the system.

This would've been ever so much more useful if it would've explained
where this magic happens.... also *urgh*.

So basically the MCE handler is doing a extable lookup on the sly to
figure out if the instruction did a user-access ? Why isn't there a
comment along with the exception crap that explains this?

Is this really the only UACCESS I lost in all that rework?

Also, MCE handler could decode the instruction and look at register
content to determine if a userspace address was involved.

> This patch works ... but to test it I had to fake out init/Kconfig so
> that it wouldn't set CONFIG_CC_HAS_ASM_GOTO_OUTPUT=y. So it seems that
> this is only needed when building with some old compiler version.

Did you do your testing on RHEL or something daft like that?

> With Linus' announcement about C99/C11 as new basis, is this fix
> needed? I.e. is it still valid to build the upstream kernel with a
> compiler that doesn't grok CONFIG_CC_HAS_ASM_GOTO_OUTPUT?

Sadly, yes, ASM_GOTO_OUTPUT is gcc-11, while we still support gcc-5.1 or
something ancient like that.

>  arch/x86/include/asm/extable_fixup_types.h |  1 +
>  arch/x86/include/asm/uaccess.h             | 15 +++++++++------
>  arch/x86/mm/extable.c                      |  8 ++++++++
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
> index 503622627400..329eeebba2f6 100644
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -30,6 +30,7 @@
>  #define EX_FLAG_CLEAR_AX		EX_DATA_FLAG(1)
>  #define EX_FLAG_CLEAR_DX		EX_DATA_FLAG(2)
>  #define EX_FLAG_CLEAR_AX_DX		EX_DATA_FLAG(3)
> +#define EX_FLAG_SET_REG		EX_DATA_FLAG(4)

That's the last available flag.. :/

Something like the below can also work, I suppose. But please, add
coherent comments to the extable code with useful references to the MCE
code that does this abuse.


diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index 503622627400..759283acb246 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -64,4 +64,7 @@
 #define	EX_TYPE_UCOPY_LEN4		(EX_TYPE_UCOPY_LEN | EX_DATA_IMM(4))
 #define	EX_TYPE_UCOPY_LEN8		(EX_TYPE_UCOPY_LEN | EX_DATA_IMM(8))
 
+#define	EX_TYPE_UA_IMM_REG		20 /* reg := (long)imm */
+#define	EX_TYPE_UFAULT_REG		(EX_TYPE_UA_IMM_REG | EX_DATA_IMM(-EFAULT))
+
 #endif
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index dba2197c05c3..b9bc0e7cb73e 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -210,6 +210,7 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 		regs->sp += sizeof(long);
 		fallthrough;
 	case EX_TYPE_IMM_REG:
+	case EX_TYPE_UA_IMM_REG:
 		return ex_handler_imm_reg(e, regs, reg, imm);
 	case EX_TYPE_FAULT_SGX:
 		return ex_handler_sgx(e, regs, trapnr);

Re: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Youquan Song]

> Did you do your testing on RHEL or something daft like that?
Tested on RHEL8.x
 
> Something like the below can also work, I suppose. But please, add
> coherent comments to the extable code with useful references to the MCE
> code that does this abuse.
Here is the full fix patch depending on your suggestion. Thanks a lot!

From 3d2e44dac774d49417002136ee3b007638a85191 Mon Sep 17 00:00:00 2001
From: Zhiquan Li <zhiquan1.li@intel.com>
Date: Wed, 30 Mar 2022 21:11:06 +0800
Subject: [PATCH v2] x86/uaccess: identify get_user with new type
 EX_TYPE_UA_IMM_REG

5.17.0 kernel will crash when we inject MCE by run "einj_mem_uc copyin"
in ras-tools with CONFIG_CC_HAS_ASM_GOTO_OUTPUT != y kernel config.
mce: [Hardware Error]: Machine check events logged
mce: [Hardware Error]: CPU 120: Machine Check Exception: f Bank 1: bd80000000100134
mce: [Hardware Error]: RIP 10: {fault_in_readable+0x9f/0xd0}
mce: [Hardware Error]: TSC 63d3fa6181b69 ADDR f921f31400 MISC 86 PPIN 11a090eb80bf0c9c
mce: [Hardware Error]: PROCESSOR 0:606a6 TIME 1647365323 SOCKET 1 APIC 8d microcode d0002e0
mce: [Hardware Error]: Run the above through 'mcelog --ascii'
mce: [Hardware Error]: Machine check: Data load in unrecoverable area of kernel
Kernel panic - not syncing: Fatal local machine check

In commit 99641e094d6c ("x86/uaccess: Remove .fixup usage"), the
exception type of get_user was changed from EX_TYPE_UACCESS to
EX_TYPE_EFAULT_REG. In case of MCE/SRAR when kernel copy data from user,
the MCE handler identities the exception type with EX_TYPE_UACCESS to
MCE_IN_KERNEL_RECOV. While the new type EX_TYPE_EFAULT_REG will lose
the opportunity to rescue the system.

To fix it we have to add a new exception type for get_user,
EX_TYPE_UA_IMM_REG, but we do not want to miss the operation that make
reg = -EFAULT in function ex_handler_imm_reg(), so we so add a new
compound type EX_TYPE_UFAULT_REG to figure out this case, it will not
break anything. Then MCE handler can identify is as a recoverable case.

Changes v1 -> v2:
* Abandoned the solution that restoring get_user exception type to
  EX_TYPE_UACCESS and adding new flag EX_FLAG_SET_REG to set
  reg := imm additionally.

Fixes: 99641e094d6c ("x86/uaccess: Remove .fixup usage")
Cc: <stable@vger.kernel.org> # v5.17+
Signed-off-by: Zhiquan Li <zhiquan1.li@intel.com>
Co-developed-by: Youquan Song <youquan.song@intel.com>
Signed-off-by: Youquan Song <youquan.song@intel.com>
Tested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/extable_fixup_types.h | 3 +++
 arch/x86/include/asm/uaccess.h             | 6 +++---
 arch/x86/kernel/cpu/mce/severity.c         | 1 +
 arch/x86/mm/extable.c                      | 1 +
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index 503622627400..759283acb246 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -64,4 +64,7 @@
 #define	EX_TYPE_UCOPY_LEN4		(EX_TYPE_UCOPY_LEN | EX_DATA_IMM(4))
 #define	EX_TYPE_UCOPY_LEN8		(EX_TYPE_UCOPY_LEN | EX_DATA_IMM(8))
 
+#define	EX_TYPE_UA_IMM_REG		20 /* reg := (long)imm */
+#define	EX_TYPE_UFAULT_REG		(EX_TYPE_UA_IMM_REG | EX_DATA_IMM(-EFAULT))
+
 #endif
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ac96f9b2d64b..2b3a4bb609fe 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -352,10 +352,10 @@ do {									\
 		     "1:	movl %[lowbits],%%eax\n"		\
 		     "2:	movl %[highbits],%%edx\n"		\
 		     "3:\n"						\
-		     _ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_EFAULT_REG |	\
+		     _ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_UFAULT_REG |	\
 					   EX_FLAG_CLEAR_AX_DX,		\
 					   %[errout])			\
-		     _ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_EFAULT_REG |	\
+		     _ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_UFAULT_REG |	\
 					   EX_FLAG_CLEAR_AX_DX,		\
 					   %[errout])			\
 		     : [errout] "=r" (retval),				\
@@ -399,7 +399,7 @@ do {									\
 	asm volatile("\n"						\
 		     "1:	mov"itype" %[umem],%[output]\n"		\
 		     "2:\n"						\
-		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG | \
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_UFAULT_REG | \
 					   EX_FLAG_CLEAR_AX,		\
 					   %[errout])			\
 		     : [errout] "=r" (err),				\
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index 7aa2bda93cbb..a20bb930b322 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -286,6 +286,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	switch (fixup_type) {
 	case EX_TYPE_UACCESS:
 	case EX_TYPE_COPY:
+	case EX_TYPE_UA_IMM_REG:
 		if (!copy_user)
 			return IN_KERNEL;
 		m->kflags |= MCE_IN_KERNEL_COPYIN;
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index dba2197c05c3..b9bc0e7cb73e 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -210,6 +210,7 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 		regs->sp += sizeof(long);
 		fallthrough;
 	case EX_TYPE_IMM_REG:
+	case EX_TYPE_UA_IMM_REG:
 		return ex_handler_imm_reg(e, regs, reg, imm);
 	case EX_TYPE_FAULT_SGX:
 		return ex_handler_sgx(e, regs, trapnr);
-- 
2.25.1

Re: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Peter Zijlstra]

On Thu, Mar 31, 2022 at 07:31:25PM +0800, Youquan Song wrote:
> > Did you do your testing on RHEL or something daft like that?
> Tested on RHEL8.x

Right; the home of obsolete software.. :-)

> > Something like the below can also work, I suppose. But please, add
> > coherent comments to the extable code with useful references to the MCE
> > code that does this abuse.
> Here is the full fix patch depending on your suggestion. Thanks a lot!

Did you verify this was indeed the only UACCESS I lost?

I'm also failing to spot a comment making extable users aware of this
abuse.

Re: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Youquan Song]

On Thu, Mar 31, 2022 at 07:51:13PM +0200, Peter Zijlstra wrote:
> On Thu, Mar 31, 2022 at 07:31:25PM +0800, Youquan Song wrote:
> > > Did you do your testing on RHEL or something daft like that?
> > Tested on RHEL8.x
> 
> Right; the home of obsolete software.. :-)
> 
> > > Something like the below can also work, I suppose. But please, add
> > > coherent comments to the extable code with useful references to the MCE
> > > code that does this abuse.
> > Here is the full fix patch depending on your suggestion. Thanks a lot!
> 
> Did you verify this was indeed the only UACCESS I lost?

The full fix patch has included a change in error_context to identify
this case to be MCE_IN_KERNEL_COPYIN. I verfied it fix the issue.
In addition, LTP was run and no issues were reported.

@@ -286,6 +286,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	switch (fixup_type) {
 	case EX_TYPE_UACCESS:
 	case EX_TYPE_COPY:
+	case EX_TYPE_UA_IMM_REG:
 		if (!copy_user)
 			return IN_KERNEL;
 		m->kflags |= MCE_IN_KERNEL_COPYIN;

-Youquan

Re: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS

[Peter Zijlstra]

On Fri, Apr 01, 2022 at 08:45:19PM +0800, Youquan Song wrote:
> On Thu, Mar 31, 2022 at 07:51:13PM +0200, Peter Zijlstra wrote:
> > On Thu, Mar 31, 2022 at 07:31:25PM +0800, Youquan Song wrote:
> > > > Did you do your testing on RHEL or something daft like that?
> > > Tested on RHEL8.x
> > 
> > Right; the home of obsolete software.. :-)
> > 
> > > > Something like the below can also work, I suppose. But please, add
> > > > coherent comments to the extable code with useful references to the MCE
> > > > code that does this abuse.
> > > Here is the full fix patch depending on your suggestion. Thanks a lot!
> > 
> > Did you verify this was indeed the only UACCESS I lost?
> 
> The full fix patch has included a change in error_context to identify
> this case to be MCE_IN_KERNEL_COPYIN. I verfied it fix the issue.

That's not what I asked... :-(

What that means is that you go look at the commits that reworked all
this; starting here:

  https://lore.kernel.org/all/20211110100102.250793167@infradead.org/

and check how many UACCESS's got removed:

$ git diff 3411506550b1..82a8954acd93 | grep "^-.*UA" | wc -l
33

And then verify they're all good now and that we don't find another
missing instance in a few weeks or later...

Because when I look at that, stuff like:

--- a/arch/x86/include/asm/futex.h
+++ b/arch/x86/include/asm/futex.h
@@ -17,13 +17,9 @@ do {                                                         \
        int oldval = 0, ret;                                    \
        asm volatile("1:\t" insn "\n"                           \
                     "2:\n"                                     \
-                    "\t.section .fixup,\"ax\"\n"               \
-                    "3:\tmov\t%3, %1\n"                        \
-                    "\tjmp\t2b\n"                              \
-                    "\t.previous\n"                            \
-                    _ASM_EXTABLE_UA(1b, 3b)                    \
+                    _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %1) \
                     : "=r" (oldval), "=r" (ret), "+m" (*uaddr) \
-                    : "i" (-EFAULT), "0" (oparg), "1" (0));    \
+                    : "0" (oparg), "1" (0));   \
        if (ret)                                                \
                goto label;                                     \
        *oval = oldval;                                         \


makes me think this isn't the last of it... Hmmm?