Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 30 Mar 2022 14:32:05 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Tony Luck <tony.luck@intel.com>
Cc:     Borislav Petkov <bp@alien8.de>,
        Josh Poimboeuf <jpoimboe@redhat.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, Zhiquan Li <zhiquan1.li@intel.com>,
        Youquan Song <youquan.song@intel.com>
Subject: Re: [PATCH] x86/uaccess: restore get_user exception type to
 EX_TYPE_UACCESS
Message-ID: <20220330123205.GL8939@worktop.programming.kicks-ass.net>
References: <20220328201748.1864491-1-tony.luck@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220328201748.1864491-1-tony.luck@intel.com>
Precedence: bulk

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);