lkml.org 
[lkml]   [2022]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] x86/uaccess: restore get_user exception type to EX_TYPE_UACCESS
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);
\
 
 \ /
  Last update: 2022-03-30 14:41    [W:0.082 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site