From: Zhiquan Li <[email protected]>
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: <[email protected]> # v5.17+
Signed-off-by: Zhiquan Li <[email protected]>
Co-developed-by: Youquan Song <[email protected]>
Signed-off-by: Youquan Song <[email protected]>
Tested-by: Tony Luck <[email protected]>
Signed-off-by: Tony Luck <[email protected]>
---
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
On Mon, Mar 28, 2022 at 01:17:48PM -0700, Tony Luck wrote:
> From: Zhiquan Li <[email protected]>
>
> 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);
> 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 <[email protected]>
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: <[email protected]> # v5.17+
Signed-off-by: Zhiquan Li <[email protected]>
Co-developed-by: Youquan Song <[email protected]>
Signed-off-by: Youquan Song <[email protected]>
Tested-by: Tony Luck <[email protected]>
Signed-off-by: Tony Luck <[email protected]>
---
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
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.
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
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/[email protected]/
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?