| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e20118fb-074e-c199-fd50-56ef1fa8ef73@linux.alibaba.com>
Date: Fri, 17 Jun 2022 10:20:09 +0800
From: Shuai Xue <xueshuai@...ux.alibaba.com>
To: Tong Tiangen <tongtiangen@...wei.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Luck, Tony" <tony.luck@...el.com>
Subject: Re: [RFC PATCH -next] arm64: ras: add uce kernel recovery support
Hi, Tiangen,
在 2022/3/23 AM11:37, Tong Tiangen 写道:
> uce means uncorrectable memory error.
>
> In the sea fault handling process of the kernel, if it is judged that the
> RAS error is consumed in the kernel, the current processing is kernel panic
> . However, it is not optimal. In some case, the page accessed in kernel is
> a user page (such as copy_from_user/get_user), in this case, kill the user
> process and isolate the user page with hardware error is a better choice.
>
> This feature provides the option. We implement it to add new extable type
> (XXX_UCE_RECOVERY), these new types indicates they can be fixup from
> uncorrectable memory error and it is fixuped in do_sea(). If the exception
> is fixuped correctly, the kernel can avoid panic.
>
> In copy_from_user, record the exception reason to regs->regs[0] at
> ex_handler_fixup_uce_recovery() which is used to check sea fault triggered.
>
> In get_user, the processing of EX_TYPE_UACCESS_ERR_ZERO and
> EX_TYPE_UACCESS_ERR_ZERO_UCE_RECOVERY is same and both return -EFAULT.
>
> Signed-off-by: Tong Tiangen <tongtiangen@...wei.com>
> ---
> arch/arm64/Kconfig | 10 ++++++++
> arch/arm64/include/asm/asm-extable.h | 35 ++++++++++++++++++++++++----
> arch/arm64/include/asm/asm-uaccess.h | 16 +++++++++++++
> arch/arm64/include/asm/esr.h | 5 ++++
> arch/arm64/include/asm/extable.h | 2 +-
> arch/arm64/include/asm/uaccess.h | 2 +-
> arch/arm64/kernel/probes/kprobes.c | 2 +-
> arch/arm64/lib/copy_from_user.S | 11 +++++----
> arch/arm64/mm/extable.c | 21 ++++++++++++++++-
> arch/arm64/mm/fault.c | 30 +++++++++++++++++++++++-
> 10 files changed, 119 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 962c84952c98..39f828c5b931 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1681,6 +1681,16 @@ config ARM64_CNP
> at runtime, and does not affect PEs that do not implement
> this feature.
>
> +config ARM64_UCE_KERNEL_RECOVERY
> + bool "Enable support for uncorrectable memory error(uce) kernel recovery"
> + default y
> + depends on ACPI_APEI_SEA
> + help
> + With ARM v8.2 RAS Extension, SEA are usually triggered when memory
> + error are consumed. In some cases, if the error address is in a
> + user page there is a chance to recover. we can isolate this page
> + and killing process instead of die.
> +
> endmenu
>
> menu "ARMv8.3 architectural features"
> diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
> index c39f2437e08e..9debab58c2b2 100644
> --- a/arch/arm64/include/asm/asm-extable.h
> +++ b/arch/arm64/include/asm/asm-extable.h
> @@ -2,11 +2,19 @@
> #ifndef __ASM_ASM_EXTABLE_H
> #define __ASM_ASM_EXTABLE_H
>
> -#define EX_TYPE_NONE 0
> -#define EX_TYPE_FIXUP 1
> -#define EX_TYPE_BPF 2
> -#define EX_TYPE_UACCESS_ERR_ZERO 3
> -#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD 4
> +#define EX_TYPE_NONE 0
> +#define EX_TYPE_FIXUP 1
> +#define EX_TYPE_BPF 2
> +#define EX_TYPE_UACCESS_ERR_ZERO 3
> +#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD 4
> +
> +/* _UCE_RECOVERY indicates that can fixup from unrecoverable memory errors */
> +#define EX_TYPE_FIXUP_UCE_RECOVERY 5
> +#define EX_TYPE_UACCESS_ERR_ZERO_UCE_RECOVERY 6
> +
> +#define IS_EX_TYPE_UCE_RECOVERY(type) \
> + (type == EX_TYPE_FIXUP_UCE_RECOVERY || \
> + type == EX_TYPE_UACCESS_ERR_ZERO_UCE_RECOVERY)
>
> #ifdef __ASSEMBLY__
>
> @@ -27,6 +35,14 @@
> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0)
> .endm
>
> +/*
> + * Create an exception table entry for `insn`, which will branch to `fixup`
> + * when an unhandled fault(include sea fault) is taken.
> + */
> + .macro _asm_extable_uce_recovery, insn, fixup
> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP_UCE_RECOVERY, 0)
> + .endm
> +
> /*
> * Create an exception table entry for `insn` if `fixup` is provided. Otherwise
> * do nothing.
> @@ -64,6 +80,15 @@
> #define EX_DATA_REG(reg, gpr) \
> "((.L__gpr_num_" #gpr ") << " __stringify(EX_DATA_REG_##reg##_SHIFT) ")"
>
> +#define _ASM_EXTABLE_UACCESS_ERR_ZERO_UCE_RECOVERY(insn, fixup, err, zero) \
> + __DEFINE_ASM_GPR_NUMS \
> + __ASM_EXTABLE_RAW(#insn, #fixup, \
> + __stringify(EX_TYPE_UACCESS_ERR_ZERO_UCE_RECOVERY), \
> + "(" \
> + EX_DATA_REG(ERR, err) " | " \
> + EX_DATA_REG(ZERO, zero) \
> + ")")
> +
> #define _ASM_EXTABLE_UACCESS_ERR_ZERO(insn, fixup, err, zero) \
> __DEFINE_ASM_GPR_NUMS \
> __ASM_EXTABLE_RAW(#insn, #fixup, \
> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
> index 0557af834e03..36692ec1fdbb 100644
> --- a/arch/arm64/include/asm/asm-uaccess.h
> +++ b/arch/arm64/include/asm/asm-uaccess.h
> @@ -92,4 +92,20 @@ alternative_else_nop_endif
>
> _asm_extable 8888b,\l;
> .endm
> +
> + .macro user_ldp_uce_recovery l, reg1, reg2, addr, post_inc
> +8888: ldtr \reg1, [\addr];
> +8889: ldtr \reg2, [\addr, #8];
> + add \addr, \addr, \post_inc;
> +
> + _asm_extable_uce_recovery 8888b, \l;
> + _asm_extable_uce_recovery 8889b, \l;
> + .endm
> +
> + .macro user_ldst_uce_recovery l, inst, reg, addr, post_inc
> +8888: \inst \reg, [\addr];
> + add \addr, \addr, \post_inc;
> +
> + _asm_extable_uce_recovery 8888b, \l;
> + .endm
> #endif
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index d52a0b269ee8..11fcfc002654 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -330,6 +330,11 @@
> #ifndef __ASSEMBLY__
> #include <asm/types.h>
>
> +static inline bool esr_is_sea(u32 esr)
> +{
> + return (esr & ESR_ELx_FSC) == ESR_ELx_FSC_EXTABT;
> +}
> +
> static inline bool esr_is_data_abort(u32 esr)
> {
> const u32 ec = ESR_ELx_EC(esr);
> diff --git a/arch/arm64/include/asm/extable.h b/arch/arm64/include/asm/extable.h
> index 72b0e71cc3de..f7835b0f473b 100644
> --- a/arch/arm64/include/asm/extable.h
> +++ b/arch/arm64/include/asm/extable.h
> @@ -45,5 +45,5 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
> }
> #endif /* !CONFIG_BPF_JIT */
>
> -bool fixup_exception(struct pt_regs *regs);
> +bool fixup_exception(struct pt_regs *regs, unsigned int esr);
> #endif
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index e8dce0cc5eaa..a229f86e6542 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -236,7 +236,7 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> asm volatile( \
> "1: " load " " reg "1, [%2]\n" \
> "2:\n" \
> - _ASM_EXTABLE_UACCESS_ERR_ZERO(1b, 2b, %w0, %w1) \
> + _ASM_EXTABLE_UACCESS_ERR_ZERO_UCE_RECOVERY(1b, 2b, %w0, %w1) \
> : "+r" (err), "=&r" (x) \
> : "r" (addr))
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d9dfa82c1f18..16a069e8eec3 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -285,7 +285,7 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> - if (fixup_exception(regs))
> + if (fixup_exception(regs, fsr))
> return 1;
> }
> return 0;
> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 34e317907524..f16104c63dde 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -21,7 +21,7 @@
> */
>
> .macro ldrb1 reg, ptr, val
> - user_ldst 9998f, ldtrb, \reg, \ptr, \val
> + user_ldst_uce_recovery 9998f, ldtrb, \reg, \ptr, \val
> .endm
>
> .macro strb1 reg, ptr, val
> @@ -29,7 +29,7 @@
> .endm
>
> .macro ldrh1 reg, ptr, val
> - user_ldst 9997f, ldtrh, \reg, \ptr, \val
> + user_ldst_uce_recovery 9997f, ldtrh, \reg, \ptr, \val
> .endm
>
> .macro strh1 reg, ptr, val
> @@ -37,7 +37,7 @@
> .endm
>
> .macro ldr1 reg, ptr, val
> - user_ldst 9997f, ldtr, \reg, \ptr, \val
> + user_ldst_uce_recovery 9997f, ldtr, \reg, \ptr, \val
> .endm
>
> .macro str1 reg, ptr, val
> @@ -45,7 +45,7 @@
> .endm
>
> .macro ldp1 reg1, reg2, ptr, val
> - user_ldp 9997f, \reg1, \reg2, \ptr, \val
> + user_ldp_uce_recovery 9997f, \reg1, \reg2, \ptr, \val
> .endm
>
> .macro stp1 reg1, reg2, ptr, val
> @@ -62,7 +62,8 @@ SYM_FUNC_START(__arch_copy_from_user)
> ret
>
> // Exception fixups
> -9997: cmp dst, dstin
> +9997: cbz x0, 9998f
> + cmp dst, dstin
> b.ne 9998f
> // Before being absolutely sure we couldn't copy anything, try harder
> USER(9998f, ldtrb tmp1w, [srcin])
> diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
> index 489455309695..b6b76e839b4b 100644
> --- a/arch/arm64/mm/extable.c
> +++ b/arch/arm64/mm/extable.c
> @@ -9,6 +9,7 @@
>
> #include <asm/asm-extable.h>
> #include <asm/ptrace.h>
> +#include <asm/esr.h>
>
> static inline unsigned long
> get_ex_fixup(const struct exception_table_entry *ex)
> @@ -23,6 +24,18 @@ static bool ex_handler_fixup(const struct exception_table_entry *ex,
> return true;
> }
>
> +static bool ex_handler_fixup_uce_recovery(const struct exception_table_entry *ex,
> + struct pt_regs *regs, unsigned int esr)
> +{
> + if (esr_is_sea(esr))
> + regs->regs[0] = 0;
> + else
> + regs->regs[0] = 1;
> +
> + regs->pc = get_ex_fixup(ex);
> + return true;
> +}
> +
> static bool ex_handler_uaccess_err_zero(const struct exception_table_entry *ex,
> struct pt_regs *regs)
> {
> @@ -63,7 +76,7 @@ ex_handler_load_unaligned_zeropad(const struct exception_table_entry *ex,
> return true;
> }
>
> -bool fixup_exception(struct pt_regs *regs)
> +bool fixup_exception(struct pt_regs *regs, unsigned int esr)
> {
> const struct exception_table_entry *ex;
>
> @@ -71,12 +84,18 @@ bool fixup_exception(struct pt_regs *regs)
> if (!ex)
> return false;
>
> + if (esr_is_sea(esr) && !IS_EX_TYPE_UCE_RECOVERY(ex->type))
> + return false;
> +
> switch (ex->type) {
> case EX_TYPE_FIXUP:
> return ex_handler_fixup(ex, regs);
> + case EX_TYPE_FIXUP_UCE_RECOVERY:
> + return ex_handler_fixup_uce_recovery(ex, regs, esr);
> case EX_TYPE_BPF:
> return ex_handler_bpf(ex, regs);
> case EX_TYPE_UACCESS_ERR_ZERO:
> + case EX_TYPE_UACCESS_ERR_ZERO_UCE_RECOVERY:
> return ex_handler_uaccess_err_zero(ex, regs);
> case EX_TYPE_LOAD_UNALIGNED_ZEROPAD:
> return ex_handler_load_unaligned_zeropad(ex, regs);
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 77341b160aca..47c447554d2a 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -361,7 +361,7 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
> * Are we prepared to handle this kernel fault?
> * We are almost certainly not prepared to handle instruction faults.
> */
> - if (!is_el1_instruction_abort(esr) && fixup_exception(regs))
> + if (!is_el1_instruction_abort(esr) && fixup_exception(regs, esr))
> return;
>
> if (WARN_RATELIMIT(is_spurious_el1_translation_fault(addr, esr, regs),
> @@ -695,6 +695,30 @@ static int do_bad(unsigned long far, unsigned int esr, struct pt_regs *regs)
> return 1; /* "fault" */
> }
>
> +static bool arm64_process_kernel_sea(unsigned long addr, unsigned int esr,
> + struct pt_regs *regs, int sig, int code)
> +{
> + if (!IS_ENABLED(CONFIG_ARM64_UCE_KERNEL_RECOVERY))
> + return false;
> +
> + if (user_mode(regs) || !current->mm)
> + return false;
> +
> + if (apei_claim_sea(regs) < 0)
> + return false;
> +
> + current->thread.fault_address = 0;
> + current->thread.fault_code = esr;
> +
> + if (!fixup_exception(regs, esr))
> + return false;
> +
> + arm64_force_sig_fault(sig, code, addr,
> + "Uncorrected hardware memory error in kernel-access\n");
> +
> + return true;
> +}
Based on X86 RAS implementation[1], I don't think we shoud use arm64_force_sig_fault to
kill process in this case. Instead, we could return -EFAULT in fixup_exception and left
the rest work in memory_failure.
> Sending a SIGBUS for a copy from user is not the correct semantic.
> System calls should return -EFAULT (or a short count for write(2)).
[1]https://lore.kernel.org/all/20210818002942.1607544-3-tony.luck@intel.com/T/#m6707a73fc8316690818b6e7eea0329b5925e1f9d
Best Regards,
Shuai
Powered by blists - more mailing lists