| 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: <ZzSh0/i0HXDnfUof@MiWiFi-R3L-srv>
Date: Wed, 13 Nov 2024 20:55:47 +0800
From: Baoquan He <bhe@...hat.com>
To: Borislav Petkov <bp@...en8.de>
Cc: Tom Lendacky <thomas.lendacky@....com>, linux-kernel@...r.kernel.org,
dyoung@...hat.com, daniel.kiper@...cle.com, noodles@...com,
lijiang@...hat.com, kexec@...ts.infradead.org, x86@...nel.org
Subject: Re: [PATCH v3 1/2] x86/mm: rename the confusing local variable in
early_memremap_is_setup_data()
On 11/06/24 at 12:20pm, Borislav Petkov wrote:
> On Sat, Nov 02, 2024 at 12:06:18PM +0100, Borislav Petkov wrote:
> > Ok, I'll take your 2/2 next week and you can then send the cleanup ontop.
>
> OMG what a mess this is. Please test the below before I apply it.
I finally got an available machine to test below patch, I can confirm
that without it the breakage can be reproduced stably; with below patch
applied the breakage is gone and vmcore dumping is successful.
>
> Then, when you do the cleanup, do the following:
Will post cleanup patch later. Thanks.
>
> - merge early_memremap_is_setup_data() with memremap_is_setup_data() into
> a common __memremap_is_setup_data() and then add a bool early which
> determines which memremap variant is called.
>
> - unify the @size argument by dropping it and using a function local size.
> What we have there now is the definition of bitrot. :-\
>
> - replace all sizeof(*data), sizeof(struct setup_data) with a macro definition
> above the functions to unify it properly.
>
> What an ugly mess... :-\
>
> ---
> From: Baoquan He <bhe@...hat.com>
> Date: Wed, 11 Sep 2024 16:16:15 +0800
> Subject: [PATCH] x86/mm: Fix a kdump kernel failure on SME system when
> CONFIG_IMA_KEXEC=y
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The kdump kernel is broken on SME systems with CONFIG_IMA_KEXEC=y enabled.
> Debugging traced the issue back to
>
> b69a2afd5afc ("x86/kexec: Carry forward IMA measurement log on kexec").
>
> Testing was previously not conducted on SME systems with CONFIG_IMA_KEXEC
> enabled, which led to the oversight, with the following incarnation:
>
> ...
> ima: No TPM chip found, activating TPM-bypass!
> Loading compiled-in module X.509 certificates
> Loaded X.509 cert 'Build time autogenerated kernel key: 18ae0bc7e79b64700122bb1d6a904b070fef2656'
> ima: Allocated hash algorithm: sha256
> Oops: general protection fault, probably for non-canonical address 0xcfacfdfe6660003e: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-rc2+ #14
> Hardware name: Dell Inc. PowerEdge R7425/02MJ3T, BIOS 1.20.0 05/03/2023
> RIP: 0010:ima_restore_measurement_list
> Call Trace:
> <TASK>
> ? show_trace_log_lvl
> ? show_trace_log_lvl
> ? ima_load_kexec_buffer
> ? __die_body.cold
> ? die_addr
> ? exc_general_protection
> ? asm_exc_general_protection
> ? ima_restore_measurement_list
> ? vprintk_emit
> ? ima_load_kexec_buffer
> ima_load_kexec_buffer
> ima_init
> ? __pfx_init_ima
> init_ima
> ? __pfx_init_ima
> do_one_initcall
> do_initcalls
> ? __pfx_kernel_init
> kernel_init_freeable
> kernel_init
> ret_from_fork
> ? __pfx_kernel_init
> ret_from_fork_asm
> </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
> ...
> Kernel panic - not syncing: Fatal exception
> Kernel Offset: disabled
> Rebooting in 10 seconds..
>
> Adding debug printks showed that the stored addr and size of ima_kexec buffer
> are not decrypted correctly like:
>
> ima: ima_load_kexec_buffer, buffer:0xcfacfdfe6660003e, size:0xe48066052d5df359
>
> Three types of setup_data info
>
> — SETUP_EFI,
> - SETUP_IMA, and
> - SETUP_RNG_SEED
>
> are passed to the kexec/kdump kernel. Only the ima_kexec buffer
> experienced incorrect decryption. Debugging identified a bug in
> early_memremap_is_setup_data(), where an incorrect range calculation
> occurred due to the len variable in struct setup_data ended up only
> representing the length of the data field, excluding the struct's size,
> and thus leading to miscalculation.
>
> Address a similar issue in memremap_is_setup_data() while at it.
>
> [ bp: Heavily massage. ]
>
> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
> Signed-off-by: Baoquan He <bhe@...hat.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>
> Acked-by: Tom Lendacky <thomas.lendacky@....com>
> Cc: <stable@...nel.org>
> Link: https://lore.kernel.org/r/20240911081615.262202-3-bhe@redhat.com
> ---
> arch/x86/mm/ioremap.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 70b02fc61d93..8d29163568a7 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -656,7 +656,8 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
> paddr_next = data->next;
> len = data->len;
>
> - if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
> + if ((phys_addr > paddr) &&
> + (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> memunmap(data);
> return true;
> }
> @@ -718,7 +719,8 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
> paddr_next = data->next;
> len = data->len;
>
> - if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
> + if ((phys_addr > paddr) &&
> + (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> early_memunmap(data, sizeof(*data));
> return true;
> }
> --
> 2.43.0
>
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
Powered by blists - more mailing lists