| 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: <4436d172-c474-8ecd-b5e4-4c21088baf49@huawei.com>
Date: Tue, 20 Aug 2024 10:11:45 +0800
From: Tong Tiangen <tongtiangen@...wei.com>
To: Mark Rutland <mark.rutland@....com>
CC: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>, James Morse <james.morse@....com>,
Robin Murphy <robin.murphy@....com>, Andrey Konovalov <andreyknvl@...il.com>,
Dmitry Vyukov <dvyukov@...gle.com>, Vincenzo Frascino
<vincenzo.frascino@....com>, Michael Ellerman <mpe@...erman.id.au>, Nicholas
Piggin <npiggin@...il.com>, Andrey Ryabinin <ryabinin.a.a@...il.com>,
Alexander Potapenko <glider@...gle.com>, Christophe Leroy
<christophe.leroy@...roup.eu>, Aneesh Kumar K.V <aneesh.kumar@...nel.org>,
"Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>, Thomas Gleixner
<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
<bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
"H. Peter Anvin" <hpa@...or.com>, <linux-arm-kernel@...ts.infradead.org>,
<linux-mm@...ck.org>, <linuxppc-dev@...ts.ozlabs.org>,
<linux-kernel@...r.kernel.org>, <wangkefeng.wang@...wei.com>, Guohanjun
<guohanjun@...wei.com>
Subject: Re: [PATCH v12 2/6] arm64: add support for ARCH_HAS_COPY_MC
在 2024/8/20 1:29, Mark Rutland 写道:
> Hi Tong,
>
> On Tue, May 28, 2024 at 04:59:11PM +0800, Tong Tiangen wrote:
>> For the arm64 kernel, when it processes hardware memory errors for
>> synchronize notifications(do_sea()), if the errors is consumed within the
>> kernel, the current processing is panic. However, it is not optimal.
>>
>> Take copy_from/to_user for example, If ld* triggers a memory error, even in
>> kernel mode, only the associated process is affected. Killing the user
>> process and isolating the corrupt page is a better choice.
>>
>> New fixup type EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE is added to identify insn
>> that can recover from memory errors triggered by access to kernel memory.
>>
>> Signed-off-by: Tong Tiangen <tongtiangen@...wei.com>
>
> Generally this looks ok, but I have a couple of comments below.
>
>> ---
>> arch/arm64/Kconfig | 1 +
>> arch/arm64/include/asm/asm-extable.h | 31 +++++++++++++++++++++++-----
>> arch/arm64/include/asm/asm-uaccess.h | 4 ++++
>> arch/arm64/include/asm/extable.h | 1 +
>> arch/arm64/lib/copy_to_user.S | 10 ++++-----
>> arch/arm64/mm/extable.c | 19 +++++++++++++++++
>> arch/arm64/mm/fault.c | 27 +++++++++++++++++-------
>> 7 files changed, 75 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 5d91259ee7b5..13ca06ddf3dd 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -20,6 +20,7 @@ config ARM64
>> select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
>> select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
>> select ARCH_HAS_CACHE_LINE_SIZE
>> + select ARCH_HAS_COPY_MC if ACPI_APEI_GHES
>> select ARCH_HAS_CURRENT_STACK_POINTER
>> select ARCH_HAS_DEBUG_VIRTUAL
>> select ARCH_HAS_DEBUG_VM_PGTABLE
>> diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
>> index 980d1dd8e1a3..9c0664fe1eb1 100644
>> --- a/arch/arm64/include/asm/asm-extable.h
>> +++ b/arch/arm64/include/asm/asm-extable.h
>> @@ -5,11 +5,13 @@
>> #include <linux/bits.h>
>> #include <asm/gpr-num.h>
>>
>> -#define EX_TYPE_NONE 0
>> -#define EX_TYPE_BPF 1
>> -#define EX_TYPE_UACCESS_ERR_ZERO 2
>> -#define EX_TYPE_KACCESS_ERR_ZERO 3
>> -#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD 4
>> +#define EX_TYPE_NONE 0
>> +#define EX_TYPE_BPF 1
>> +#define EX_TYPE_UACCESS_ERR_ZERO 2
>> +#define EX_TYPE_KACCESS_ERR_ZERO 3
>> +#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD 4
>> +/* kernel access memory error safe */
>> +#define EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE 5
>
> Could we please use 'MEM_ERR', and likewise for the macros below? That's
> more obvious than 'ME_SAFE', and we wouldn't need the comment here.
> Likewise elsewhere in this patch and the series.
>
> To Jonathan's comment, I do prefer these numbers are aligned, so aside
> from the naming, the diff above looks good.
OK, I also modified other locations to use 'MEM_ERR'.
>
>>
>> /* Data fields for EX_TYPE_UACCESS_ERR_ZERO */
>> #define EX_DATA_REG_ERR_SHIFT 0
>> @@ -51,6 +53,17 @@
>> #define _ASM_EXTABLE_UACCESS(insn, fixup) \
>> _ASM_EXTABLE_UACCESS_ERR_ZERO(insn, fixup, wzr, wzr)
>>
>> +#define _ASM_EXTABLE_KACCESS_ERR_ZERO_ME_SAFE(insn, fixup, err, zero) \
>> + __ASM_EXTABLE_RAW(insn, fixup, \
>> + EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE, \
>> + ( \
>> + EX_DATA_REG(ERR, err) | \
>> + EX_DATA_REG(ZERO, zero) \
>> + ))
>> +
>> +#define _ASM_EXTABLE_KACCESS_ME_SAFE(insn, fixup) \
>> + _ASM_EXTABLE_KACCESS_ERR_ZERO_ME_SAFE(insn, fixup, wzr, wzr)
>> +
>> /*
>> * Create an exception table entry for uaccess `insn`, which will branch to `fixup`
>> * when an unhandled fault is taken.
>> @@ -69,6 +82,14 @@
>> .endif
>> .endm
>>
>> +/*
>> + * Create an exception table entry for kaccess me(memory error) safe `insn`, which
>> + * will branch to `fixup` when an unhandled fault is taken.
>> + */
>> + .macro _asm_extable_kaccess_me_safe, insn, fixup
>> + _ASM_EXTABLE_KACCESS_ME_SAFE(\insn, \fixup)
>> + .endm
>> +
>
> With the naming above, I think this can be:
>
> | /*
> | * Create an exception table entry for kaccess `insn`, which will branch to
> | * `fixup` when a memory error is taken
> | */
> | .macro _asm_extable_kaccess_mem_err, insn, fixup
> | _ASM_EXTABLE_KACCESS_MEM_ERR(\insn, \fixup)
> | .endm
>
OK, will be fixed next version.
>> #else /* __ASSEMBLY__ */
>>
>> #include <linux/stringify.h>
>> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
>> index 5b6efe8abeeb..7bbebfa5b710 100644
>> --- a/arch/arm64/include/asm/asm-uaccess.h
>> +++ b/arch/arm64/include/asm/asm-uaccess.h
>> @@ -57,6 +57,10 @@ alternative_else_nop_endif
>> .endm
>> #endif
>>
>> +#define KERNEL_ME_SAFE(l, x...) \
>> +9999: x; \
>> + _asm_extable_kaccess_me_safe 9999b, l
>> +
>> #define USER(l, x...) \
>> 9999: x; \
>> _asm_extable_uaccess 9999b, l
>> diff --git a/arch/arm64/include/asm/extable.h b/arch/arm64/include/asm/extable.h
>> index 72b0e71cc3de..bc49443bc502 100644
>> --- a/arch/arm64/include/asm/extable.h
>> +++ b/arch/arm64/include/asm/extable.h
>> @@ -46,4 +46,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_me(struct pt_regs *regs);
>> #endif
>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>> index 802231772608..2ac716c0d6d8 100644
>> --- a/arch/arm64/lib/copy_to_user.S
>> +++ b/arch/arm64/lib/copy_to_user.S
>> @@ -20,7 +20,7 @@
>> * x0 - bytes not copied
>> */
>> .macro ldrb1 reg, ptr, val
>> - ldrb \reg, [\ptr], \val
>> + KERNEL_ME_SAFE(9998f, ldrb \reg, [\ptr], \val)
>> .endm
>>
>> .macro strb1 reg, ptr, val
>> @@ -28,7 +28,7 @@
>> .endm
>>
>> .macro ldrh1 reg, ptr, val
>> - ldrh \reg, [\ptr], \val
>> + KERNEL_ME_SAFE(9998f, ldrh \reg, [\ptr], \val)
>> .endm
>>
>> .macro strh1 reg, ptr, val
>> @@ -36,7 +36,7 @@
>> .endm
>>
>> .macro ldr1 reg, ptr, val
>> - ldr \reg, [\ptr], \val
>> + KERNEL_ME_SAFE(9998f, ldr \reg, [\ptr], \val)
>> .endm
>>
>> .macro str1 reg, ptr, val
>> @@ -44,7 +44,7 @@
>> .endm
>>
>> .macro ldp1 reg1, reg2, ptr, val
>> - ldp \reg1, \reg2, [\ptr], \val
>> + KERNEL_ME_SAFE(9998f, ldp \reg1, \reg2, [\ptr], \val)
>> .endm
>>
>> .macro stp1 reg1, reg2, ptr, val
>
> These changes mean that regular copy_to_user() will handle kernel memory
> errors, rather than only doing that in copy_mc_to_user(). If that's
> intentional, please call that out explicitly in the commit message.
Yes. This is the purpose of the modification. If the copy_to_user()
function encounters a memory error, this uaccess affects only the
current process. and only need to kill the current process instead of
the entire kernel panic. Do not add copy_mc_to_user() so that
copy_to_user() can process memory errors.
I'll add a description in the commit msg next version.
>
>> @@ -64,7 +64,7 @@ SYM_FUNC_START(__arch_copy_to_user)
>> 9997: cmp dst, dstin
>> b.ne 9998f
>> // Before being absolutely sure we couldn't copy anything, try harder
>> - ldrb tmp1w, [srcin]
>> +KERNEL_ME_SAFE(9998f, ldrb tmp1w, [srcin])
>> USER(9998f, sttrb tmp1w, [dst])
>> add dst, dst, #1
>> 9998: sub x0, end, dst // bytes not copied
>
> Same comment as above.
>
>> diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
>> index 228d681a8715..8c690ae61944 100644
>> --- a/arch/arm64/mm/extable.c
>> +++ b/arch/arm64/mm/extable.c
>> @@ -72,7 +72,26 @@ bool fixup_exception(struct pt_regs *regs)
>> return ex_handler_uaccess_err_zero(ex, regs);
>> case EX_TYPE_LOAD_UNALIGNED_ZEROPAD:
>> return ex_handler_load_unaligned_zeropad(ex, regs);
>> + case EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE:
>> + return false;
>> }
>>
>> BUG();
>> }
>> +
>> +bool fixup_exception_me(struct pt_regs *regs)
>> +{
>> + const struct exception_table_entry *ex;
>> +
>> + ex = search_exception_tables(instruction_pointer(regs));
>> + if (!ex)
>> + return false;
>> +
>> + switch (ex->type) {
>> + case EX_TYPE_UACCESS_ERR_ZERO:
>> + case EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE:
>> + return ex_handler_uaccess_err_zero(ex, regs);
>> + }
>> +
>> + return false;
>> +}
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 451ba7cbd5ad..2dc65f99d389 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -708,21 +708,32 @@ static int do_bad(unsigned long far, unsigned long esr, struct pt_regs *regs)
>> return 1; /* "fault" */
>> }
>>
>> +/*
>> + * APEI claimed this as a firmware-first notification.
>> + * Some processing deferred to task_work before ret_to_user().
>> + */
>> +static bool do_apei_claim_sea(struct pt_regs *regs)
>> +{
>> + if (user_mode(regs)) {
>> + if (!apei_claim_sea(regs))
>> + return true;
>> + } else if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) {
>> + if (fixup_exception_me(regs) && !apei_claim_sea(regs))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>
> Hmm... that'll fixup the exception even if we don't manage to claim a
> the SEA. I suspect this should probably be:
>
> static bool do_apei_claim_sea(struct pt_regs *regs)
> {
> if (apei_claim_sea(regs))
> return false;
> if (user_mode(regs))
> return true;
> if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC))
> return !fixup_excepton_mem_err(regs);
>
> return false;
> }
>
> ... unless we *don't* want to claim the SEA in the case we don't have a
> fixup?
>
> Mark.
>
Yes. My original meaning here is that if not have fixup, panic is
performed in do_sea() according to the original logic, and claim sea is
not required.
Thanks,
Tong.
>> +
>> static int do_sea(unsigned long far, unsigned long esr, struct pt_regs *regs)
>> {
>> const struct fault_info *inf;
>> unsigned long siaddr;
>>
>> - inf = esr_to_fault_info(esr);
>> -
>> - if (user_mode(regs) && apei_claim_sea(regs) == 0) {
>> - /*
>> - * APEI claimed this as a firmware-first notification.
>> - * Some processing deferred to task_work before ret_to_user().
>> - */
>> + if (do_apei_claim_sea(regs))
>> return 0;
>> - }
>>
>> + inf = esr_to_fault_info(esr);
>> if (esr & ESR_ELx_FnV) {
>> siaddr = 0;
>> } else {
>> --
>> 2.25.1
>>
>>
>
> .
Powered by blists - more mailing lists