public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y
@ 2024-08-29 10:40 Baoquan He
  2024-08-29 10:40 ` [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data() Baoquan He
  2024-08-29 10:40 ` [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
  0 siblings, 2 replies; 6+ messages in thread
From: Baoquan He @ 2024-08-29 10:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: thomas.lendacky, dyoung, daniel.kiper, noodles, lijiang, kexec,
	Baoquan He

In this v2 posting,
- patch 2 is to fix the kdump kernel breakage;
- patch 1 is added to clean up the confusing local varibale naming because
people may mix up the local variable 'size' with the passed in parameter
in function early_memremap_is_setup_data(). This cleanup is suggested by
Dave and Tom during v1 patch reviewing.

V1 post can be found here:
===
[PATCH] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y
https://lore.kernel.org/all/20240826024457.22423-1-bhe@redhat.com/T/#u

Baoquan He (2):
  x86/mm: rename the confusing local variable in
    early_memremap_is_setup_data()
  x86/mm/sme: fix the kdump kernel breakage on SME system when
    CONFIG_IMA_KEXEC=y

 arch/x86/mm/ioremap.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.41.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data()
  2024-08-29 10:40 [PATCH v2 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
@ 2024-08-29 10:40 ` Baoquan He
  2024-09-09 14:43   ` Tom Lendacky
  2024-08-29 10:40 ` [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
  1 sibling, 1 reply; 6+ messages in thread
From: Baoquan He @ 2024-08-29 10:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: thomas.lendacky, dyoung, daniel.kiper, noodles, lijiang, kexec,
	Baoquan He

In function early_memremap_is_setup_data(), parameter 'size' passed has
the same name as the local variable inside the while loop. That
confuses people who sometime mix up them when reading code.

Here rename the local variable 'size' inside while loop to 'sd_size'.

And also add one local variable 'sd_size' likewise in function
memremap_is_setup_data() to simplify code. In later patch, this can also
be used.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/mm/ioremap.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index aa7d279321ea..f1ee8822ddf1 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -640,7 +640,7 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
 
 	paddr = boot_params.hdr.setup_data;
 	while (paddr) {
-		unsigned int len;
+		unsigned int len, sd_size;
 
 		if (phys_addr == paddr)
 			return true;
@@ -652,6 +652,8 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
 			return false;
 		}
 
+		sd_size = sizeof(*data);
+
 		paddr_next = data->next;
 		len = data->len;
 
@@ -662,7 +664,9 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
 
 		if (data->type == SETUP_INDIRECT) {
 			memunmap(data);
-			data = memremap(paddr, sizeof(*data) + len,
+
+			sd_size += len;
+			data = memremap(paddr, sd_size,
 					MEMREMAP_WB | MEMREMAP_DEC);
 			if (!data) {
 				pr_warn("failed to memremap indirect setup_data\n");
@@ -701,7 +705,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
 
 	paddr = boot_params.hdr.setup_data;
 	while (paddr) {
-		unsigned int len, size;
+		unsigned int len, sd_size;
 
 		if (phys_addr == paddr)
 			return true;
@@ -712,7 +716,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
 			return false;
 		}
 
-		size = sizeof(*data);
+		sd_size = sizeof(*data);
 
 		paddr_next = data->next;
 		len = data->len;
@@ -723,9 +727,9 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
 		}
 
 		if (data->type == SETUP_INDIRECT) {
-			size += len;
+			sd_size += len;
 			early_memunmap(data, sizeof(*data));
-			data = early_memremap_decrypted(paddr, size);
+			data = early_memremap_decrypted(paddr, sd_size);
 			if (!data) {
 				pr_warn("failed to early memremap indirect setup_data\n");
 				return false;
@@ -739,7 +743,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
 			}
 		}
 
-		early_memunmap(data, size);
+		early_memunmap(data, sd_size);
 
 		if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
 			return true;
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y
  2024-08-29 10:40 [PATCH v2 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
  2024-08-29 10:40 ` [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data() Baoquan He
@ 2024-08-29 10:40 ` Baoquan He
  2024-09-09 14:53   ` Tom Lendacky
  1 sibling, 1 reply; 6+ messages in thread
From: Baoquan He @ 2024-08-29 10:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: thomas.lendacky, dyoung, daniel.kiper, noodles, lijiang, kexec,
	Baoquan He

Recently, it's reported that kdump kernel is broken during bootup on
SME system when CONFIG_IMA_KEXEC=y. When debugging, I noticed this
can be traced back to commit ("b69a2afd5afc x86/kexec: Carry forward
IMA measurement log on kexec"). Just nobody ever tested it on SME
system when enabling CONFIG_IMA_KEXEC.

--------------------------------------------------
 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+0xdc/0x420
 Code: ff 48 c7 85 10 ff ff ff 00 00 00 00 48 c7 85 18 ff ff ff 00 00 00 00 48 85 f6 0f 84 09 03 00 00 48 83 fa 17 0f 86 ff 02 00 00 <66> 83 3e 01 49 89 f4 0f 85 90 94 7d 00 48 83 7e 10 ff 0f 84 74 94
 RSP: 0018:ffffc90000053c80 EFLAGS: 00010286
 RAX: 0000000000000000 RBX: ffffc90000053d03 RCX: 0000000000000000
 RDX: e48066052d5df359 RSI: cfacfdfe6660003e RDI: cfacfdfe66600056
 RBP: ffffc90000053d80 R08: 0000000000000000 R09: ffffffff82de1a88
 R10: ffffc90000053da0 R11: 0000000000000003 R12: 00000000000001a4
 R13: ffffc90000053df0 R14: 0000000000000000 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff888040200000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f2c744050e8 CR3: 000080004110e000 CR4: 00000000003506b0
 Call Trace:
  <TASK>
  ? show_trace_log_lvl+0x1b0/0x2f0
  ? show_trace_log_lvl+0x1b0/0x2f0
  ? ima_load_kexec_buffer+0x6e/0xf0
  ? __die_body.cold+0x8/0x12
  ? die_addr+0x3c/0x60
  ? exc_general_protection+0x178/0x410
  ? asm_exc_general_protection+0x26/0x30
  ? ima_restore_measurement_list+0xdc/0x420
  ? vprintk_emit+0x1f0/0x270
  ? ima_load_kexec_buffer+0x6e/0xf0
  ima_load_kexec_buffer+0x6e/0xf0
  ima_init+0x52/0xb0
  ? __pfx_init_ima+0x10/0x10
  init_ima+0x26/0xc0
  ? __pfx_init_ima+0x10/0x10
  do_one_initcall+0x5b/0x300
  do_initcalls+0xdf/0x100
  ? __pfx_kernel_init+0x10/0x10
  kernel_init_freeable+0x147/0x1a0
  kernel_init+0x1a/0x140
  ret_from_fork+0x34/0x50
  ? __pfx_kernel_init+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
  </TASK>
 Modules linked in:
 ---[ end trace 0000000000000000 ]---
 RIP: 0010:ima_restore_measurement_list+0xdc/0x420
 Code: ff 48 c7 85 10 ff ff ff 00 00 00 00 48 c7 85 18 ff ff ff 00 00 00 00 48 85 f6 0f 84 09 03 00 00 48 83 fa 17 0f 86 ff 02 00 00 <66> 83 3e 01 49 89 f4 0f 85 90 94 7d 00 48 83 7e 10 ff 0f 84 74 94
 RSP: 0018:ffffc90000053c80 EFLAGS: 00010286
 RAX: 0000000000000000 RBX: ffffc90000053d03 RCX: 0000000000000000
 RDX: e48066052d5df359 RSI: cfacfdfe6660003e RDI: cfacfdfe66600056
 RBP: ffffc90000053d80 R08: 0000000000000000 R09: ffffffff82de1a88
 R10: ffffc90000053da0 R11: 0000000000000003 R12: 00000000000001a4
 R13: ffffc90000053df0 R14: 0000000000000000 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff888040200000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f2c744050e8 CR3: 000080004110e000 CR4: 00000000003506b0
 Kernel panic - not syncing: Fatal exception
 Kernel Offset: disabled
 Rebooting in 10 seconds..

From debugging printing, the stored addr and size of ima_kexec buffer
are not decrypted correctly like:
 ------
 ima: ima_load_kexec_buffer, buffer:0xcfacfdfe6660003e, size:0xe48066052d5df359
 ------

There are three pieces of setup_data info passed to kexec/kdump kernel:
SETUP_EFI, SETUP_IMA and SETUP_RNG_SEED. However, among them, only
ima_kexec buffer suffered from the incorrect decryption. After
debugging, it's because of the code bug in early_memremap_is_setup_data()
where checking the embedded content inside setup_data takes wrong range
calculation.

The length of efi data, rng_seed and ima_kexec are 0x70, 0x20, 0x10,
and the length of setup_data is 0x10. When checking if data is inside
the embedded conent of setup_data, the starting address of efi data and
rng_seed happened to land in the wrong calculated range. While the
ima_kexec's starting address unluckily doesn't pass the checking, then
error occurred.

Here fix the code bug to make kexec/kdump kernel boot up successfully.

And also fix the similar buggy code in memremap_is_setup_data() which
are found out during code reviewing.

Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/mm/ioremap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index f1ee8822ddf1..4cadc7ef1cb4 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -657,7 +657,7 @@ 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 + sd_size + len))) {
 			memunmap(data);
 			return true;
 		}
@@ -721,7 +721,7 @@ 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 + sd_size + len))) {
 			early_memunmap(data, sizeof(*data));
 			return true;
 		}
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data()
  2024-08-29 10:40 ` [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data() Baoquan He
@ 2024-09-09 14:43   ` Tom Lendacky
  0 siblings, 0 replies; 6+ messages in thread
From: Tom Lendacky @ 2024-09-09 14:43 UTC (permalink / raw)
  To: Baoquan He, linux-kernel; +Cc: dyoung, daniel.kiper, noodles, lijiang, kexec

On 8/29/24 05:40, Baoquan He wrote:
> In function early_memremap_is_setup_data(), parameter 'size' passed has
> the same name as the local variable inside the while loop. That
> confuses people who sometime mix up them when reading code.
> 
> Here rename the local variable 'size' inside while loop to 'sd_size'.
> 
> And also add one local variable 'sd_size' likewise in function
> memremap_is_setup_data() to simplify code. In later patch, this can also
> be used.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/mm/ioremap.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index aa7d279321ea..f1ee8822ddf1 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -640,7 +640,7 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  
>  	paddr = boot_params.hdr.setup_data;
>  	while (paddr) {
> -		unsigned int len;
> +		unsigned int len, sd_size;
>  
>  		if (phys_addr == paddr)
>  			return true;
> @@ -652,6 +652,8 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  			return false;
>  		}
>  
> +		sd_size = sizeof(*data);
> +
>  		paddr_next = data->next;
>  		len = data->len;
>  
> @@ -662,7 +664,9 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  
>  		if (data->type == SETUP_INDIRECT) {
>  			memunmap(data);
> -			data = memremap(paddr, sizeof(*data) + len,
> +
> +			sd_size += len;
> +			data = memremap(paddr, sd_size,
>  					MEMREMAP_WB | MEMREMAP_DEC);
>  			if (!data) {
>  				pr_warn("failed to memremap indirect setup_data\n");
> @@ -701,7 +705,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>  
>  	paddr = boot_params.hdr.setup_data;
>  	while (paddr) {
> -		unsigned int len, size;
> +		unsigned int len, sd_size;
>  
>  		if (phys_addr == paddr)
>  			return true;
> @@ -712,7 +716,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>  			return false;
>  		}
>  
> -		size = sizeof(*data);
> +		sd_size = sizeof(*data);
>  
>  		paddr_next = data->next;
>  		len = data->len;
> @@ -723,9 +727,9 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>  		}
>  
>  		if (data->type == SETUP_INDIRECT) {
> -			size += len;
> +			sd_size += len;
>  			early_memunmap(data, sizeof(*data));
> -			data = early_memremap_decrypted(paddr, size);
> +			data = early_memremap_decrypted(paddr, sd_size);
>  			if (!data) {
>  				pr_warn("failed to early memremap indirect setup_data\n");
>  				return false;
> @@ -739,7 +743,7 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>  			}
>  		}
>  
> -		early_memunmap(data, size);
> +		early_memunmap(data, sd_size);
>  
>  		if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
>  			return true;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y
  2024-08-29 10:40 ` [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
@ 2024-09-09 14:53   ` Tom Lendacky
  2024-09-10  9:40     ` Baoquan He
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Lendacky @ 2024-09-09 14:53 UTC (permalink / raw)
  To: Baoquan He, linux-kernel; +Cc: dyoung, daniel.kiper, noodles, lijiang, kexec

On 8/29/24 05:40, Baoquan He wrote:
> Recently, it's reported that kdump kernel is broken during bootup on
> SME system when CONFIG_IMA_KEXEC=y. When debugging, I noticed this
> can be traced back to commit ("b69a2afd5afc x86/kexec: Carry forward
> IMA measurement log on kexec"). Just nobody ever tested it on SME
> system when enabling CONFIG_IMA_KEXEC.
> 
> --------------------------------------------------
>  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+0xdc/0x420
>  Code: ff 48 c7 85 10 ff ff ff 00 00 00 00 48 c7 85 18 ff ff ff 00 00 00 00 48 85 f6 0f 84 09 03 00 00 48 83 fa 17 0f 86 ff 02 00 00 <66> 83 3e 01 49 89 f4 0f 85 90 94 7d 00 48 83 7e 10 ff 0f 84 74 94
>  RSP: 0018:ffffc90000053c80 EFLAGS: 00010286
>  RAX: 0000000000000000 RBX: ffffc90000053d03 RCX: 0000000000000000
>  RDX: e48066052d5df359 RSI: cfacfdfe6660003e RDI: cfacfdfe66600056
>  RBP: ffffc90000053d80 R08: 0000000000000000 R09: ffffffff82de1a88
>  R10: ffffc90000053da0 R11: 0000000000000003 R12: 00000000000001a4
>  R13: ffffc90000053df0 R14: 0000000000000000 R15: 0000000000000000
>  FS:  0000000000000000(0000) GS:ffff888040200000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f2c744050e8 CR3: 000080004110e000 CR4: 00000000003506b0
>  Call Trace:
>   <TASK>
>   ? show_trace_log_lvl+0x1b0/0x2f0
>   ? show_trace_log_lvl+0x1b0/0x2f0
>   ? ima_load_kexec_buffer+0x6e/0xf0
>   ? __die_body.cold+0x8/0x12
>   ? die_addr+0x3c/0x60
>   ? exc_general_protection+0x178/0x410
>   ? asm_exc_general_protection+0x26/0x30
>   ? ima_restore_measurement_list+0xdc/0x420
>   ? vprintk_emit+0x1f0/0x270
>   ? ima_load_kexec_buffer+0x6e/0xf0
>   ima_load_kexec_buffer+0x6e/0xf0
>   ima_init+0x52/0xb0
>   ? __pfx_init_ima+0x10/0x10
>   init_ima+0x26/0xc0
>   ? __pfx_init_ima+0x10/0x10
>   do_one_initcall+0x5b/0x300
>   do_initcalls+0xdf/0x100
>   ? __pfx_kernel_init+0x10/0x10
>   kernel_init_freeable+0x147/0x1a0
>   kernel_init+0x1a/0x140
>   ret_from_fork+0x34/0x50
>   ? __pfx_kernel_init+0x10/0x10
>   ret_from_fork_asm+0x1a/0x30
>   </TASK>
>  Modules linked in:
>  ---[ end trace 0000000000000000 ]---
>  RIP: 0010:ima_restore_measurement_list+0xdc/0x420
>  Code: ff 48 c7 85 10 ff ff ff 00 00 00 00 48 c7 85 18 ff ff ff 00 00 00 00 48 85 f6 0f 84 09 03 00 00 48 83 fa 17 0f 86 ff 02 00 00 <66> 83 3e 01 49 89 f4 0f 85 90 94 7d 00 48 83 7e 10 ff 0f 84 74 94
>  RSP: 0018:ffffc90000053c80 EFLAGS: 00010286
>  RAX: 0000000000000000 RBX: ffffc90000053d03 RCX: 0000000000000000
>  RDX: e48066052d5df359 RSI: cfacfdfe6660003e RDI: cfacfdfe66600056
>  RBP: ffffc90000053d80 R08: 0000000000000000 R09: ffffffff82de1a88
>  R10: ffffc90000053da0 R11: 0000000000000003 R12: 00000000000001a4
>  R13: ffffc90000053df0 R14: 0000000000000000 R15: 0000000000000000
>  FS:  0000000000000000(0000) GS:ffff888040200000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f2c744050e8 CR3: 000080004110e000 CR4: 00000000003506b0
>  Kernel panic - not syncing: Fatal exception
>  Kernel Offset: disabled
>  Rebooting in 10 seconds..
> 
> From debugging printing, the stored addr and size of ima_kexec buffer
> are not decrypted correctly like:
>  ------
>  ima: ima_load_kexec_buffer, buffer:0xcfacfdfe6660003e, size:0xe48066052d5df359
>  ------
> 
> There are three pieces of setup_data info passed to kexec/kdump kernel:
> SETUP_EFI, SETUP_IMA and SETUP_RNG_SEED. However, among them, only
> ima_kexec buffer suffered from the incorrect decryption. After
> debugging, it's because of the code bug in early_memremap_is_setup_data()
> where checking the embedded content inside setup_data takes wrong range
> calculation.
> 
> The length of efi data, rng_seed and ima_kexec are 0x70, 0x20, 0x10,
> and the length of setup_data is 0x10. When checking if data is inside
> the embedded conent of setup_data, the starting address of efi data and
> rng_seed happened to land in the wrong calculated range. While the
> ima_kexec's starting address unluckily doesn't pass the checking, then
> error occurred.
> 
> Here fix the code bug to make kexec/kdump kernel boot up successfully.
> 
> And also fix the similar buggy code in memremap_is_setup_data() which
> are found out during code reviewing.

I think you should add something along the lines that the "len" variable
in struct setup_data is the length of the "data" field and does not
include the size of the struct, which is the reason for the miscalculation.

Otherwise:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 
> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/mm/ioremap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index f1ee8822ddf1..4cadc7ef1cb4 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -657,7 +657,7 @@ 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 + sd_size + len))) {
>  			memunmap(data);
>  			return true;
>  		}
> @@ -721,7 +721,7 @@ 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 + sd_size + len))) {
>  			early_memunmap(data, sizeof(*data));
>  			return true;
>  		}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y
  2024-09-09 14:53   ` Tom Lendacky
@ 2024-09-10  9:40     ` Baoquan He
  0 siblings, 0 replies; 6+ messages in thread
From: Baoquan He @ 2024-09-10  9:40 UTC (permalink / raw)
  To: Tom Lendacky; +Cc: linux-kernel, dyoung, daniel.kiper, noodles, lijiang, kexec

On 09/09/24 at 09:53am, Tom Lendacky wrote:
> On 8/29/24 05:40, Baoquan He wrote:
......snip.. 
> > Here fix the code bug to make kexec/kdump kernel boot up successfully.
> > 
> > And also fix the similar buggy code in memremap_is_setup_data() which
> > are found out during code reviewing.
> 
> I think you should add something along the lines that the "len" variable
> in struct setup_data is the length of the "data" field and does not
> include the size of the struct, which is the reason for the miscalculation.

Fair enough. I will send v3 to add the reason of miscalculation.

> 
> Otherwise:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

Thanks a lot for careful checking.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-09-10  9:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-29 10:40 [PATCH v2 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
2024-08-29 10:40 ` [PATCH v2 1/2] x86/mm: rename the confusing local variable in early_memremap_is_setup_data() Baoquan He
2024-09-09 14:43   ` Tom Lendacky
2024-08-29 10:40 ` [PATCH v2 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y Baoquan He
2024-09-09 14:53   ` Tom Lendacky
2024-09-10  9:40     ` Baoquan He

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox