Re: [PATCH v3 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data()

[Baoquan He <bhe@redhat.com>]

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@redhat.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@redhat.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: <stable@kernel.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
>