| 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: <20240819113032.000042af@Huawei.com>
Date: Mon, 19 Aug 2024 11:30:32 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Tong Tiangen <tongtiangen@...wei.com>
CC: Mark Rutland <mark.rutland@....com>, 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
On Tue, 28 May 2024 16:59:11 +0800
Tong Tiangen <tongtiangen@...wei.com> 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>
Hi - this is going slow :(
A few comments inline in the meantime but this really needs ARM maintainers
to take a (hopefully final) look.
Jonathan
> 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
Does anyone care enough about the alignment to bother realigning for one
long line? I'd be tempted not to bother, but up to maintainers.
> 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
> @@ -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])
Alignment looks off?
> USER(9998f, sttrb tmp1w, [dst])
> add dst, dst, #1
> 9998: sub x0, end, dst // bytes not copied
> 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))
I'd keep to the the (apei_claim_sea(regs) == 0)
used in the original code. That hints to the reader that we are
interested here in an 'error' code rather than apei_claim_sea() returning
a bool. I initially wondered why we return true when the code
fails to claim it.
Also, perhaps if you return 0 for success and an error code if not
you could just make this
if (user_mode(regs))
return apei_claim_sea(regs);
if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) {
if (fixup_exception_me(regs)) {
return apei_claim_sea(regs);
}
}
return false;
or maybe even (I may have messed this up, but I think this logic
works).
if (!user_mode(regs) && IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) {
if (!fixup_exception_me(regs))
return false;
}
return apei_claim_sea(regs);
> + return true;
> + } else if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) {
> + if (fixup_exception_me(regs) && !apei_claim_sea(regs))
Same here with using apei_claim_sea(regs) == 0 so it's obvious we
are checking for an error, not a boolean.
> + return true;
> + }
> +
> + return false;
> +}
> +
> 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))
It might be made sense to factor this out first, then could be reviewed
as a noop before the new stuff is added. Still it's not much code, so doesn't
really matter.
Might be worth keeping to returning 0 for success, error code
otherwise as per apei_claim_sea(regs)
The bool returning functions in the nearby code tend to be is_xxxx
not things that succeed or not.
If you change it to return int make this
if (do_apei_claim_sea(regs) == 0)
so it's obvious this is the no error case.
> return 0;
> - }
>
> + inf = esr_to_fault_info(esr);
> if (esr & ESR_ELx_FnV) {
> siaddr = 0;
> } else {
Powered by blists - more mailing lists