[PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Baoquan He]

Currently, distros like Fedora/RHEL have enabled CONFIG_IMA_KEXEC by
default. This makes kexec/kdump kernel always fail to boot up on SME
platform because of a code bug. By debugging, the root cause is found
out and bug is fixed with this patchset.

Changelog:
v2->v3:
=======
- Add how the miscaculation is caused into patch 2 log according to
  Tom's suggestion.
- Add Tom's tag.

v1->v2:
=======
- Add patch 1 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(). Suggested by Dave and Tom
  during v1 patch reviewing.

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

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

[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>
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;
-- 
2.41.0

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

[Borislav Petkov]

On Wed, Sep 11, 2024 at 04:16:14PM +0800, 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,

Huh?

---
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 70b02fc61d93..e461d8e26871 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -632,8 +632,7 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
  * Examine the physical address to determine if it is boot data by checking
  * it against the boot params setup_data chain.
  */
-static bool memremap_is_setup_data(resource_size_t phys_addr,
-				   unsigned long size)
+static bool memremap_is_setup_data(resource_size_t phys_addr)
 {
 	struct setup_indirect *indirect;
 	struct setup_data *data;
@@ -769,7 +768,7 @@ bool arch_memremap_can_ram_remap(resource_size_t phys_addr, unsigned long size,
 		return false;
 
 	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
-		if (memremap_is_setup_data(phys_addr, size) ||
+		if (memremap_is_setup_data(phys_addr) ||
 		    memremap_is_efi_data(phys_addr, size))
 			return false;
 	}

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Baoquan He]

On 10/29/24 at 07:11pm, Borislav Petkov wrote:
> On Wed, Sep 11, 2024 at 04:16:14PM +0800, 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,
> 
> Huh?

Thanks for looking into this.

I ever doubted this, guess it could use the unused 'size' to avoid
warning? Noticed Tom introduced it at the beginning. It's better idea to
remove it if it's useless.

commit 8f716c9b5febf6ed0f5fedb7c9407cd0c25b2796
Author: Tom Lendacky <thomas.lendacky@amd.com>
Date:   Mon Jul 17 16:10:16 2017 -0500

    x86/mm: Add support to access boot related data in the clear

Hi Tom,

Can you help check and tell your intention why the argument 'size' is
added into early_memremap_is_setup_data() and memremap_is_setup_data().

Thanks
Baoquan

> 
> ---
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 70b02fc61d93..e461d8e26871 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -632,8 +632,7 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
>   * Examine the physical address to determine if it is boot data by checking
>   * it against the boot params setup_data chain.
>   */
> -static bool memremap_is_setup_data(resource_size_t phys_addr,
> -				   unsigned long size)
> +static bool memremap_is_setup_data(resource_size_t phys_addr)
>  {
>  	struct setup_indirect *indirect;
>  	struct setup_data *data;
> @@ -769,7 +768,7 @@ bool arch_memremap_can_ram_remap(resource_size_t phys_addr, unsigned long size,
>  		return false;
>  
>  	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> -		if (memremap_is_setup_data(phys_addr, size) ||
> +		if (memremap_is_setup_data(phys_addr) ||
>  		    memremap_is_efi_data(phys_addr, size))
>  			return false;
>  	}
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

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

[Tom Lendacky]

On 10/29/24 19:53, Baoquan He wrote:
> On 10/29/24 at 07:11pm, Borislav Petkov wrote:
>> On Wed, Sep 11, 2024 at 04:16:14PM +0800, 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,
>>
>> Huh?
> 
> Thanks for looking into this.
> 
> I ever doubted this, guess it could use the unused 'size' to avoid
> warning? Noticed Tom introduced it at the beginning. It's better idea to
> remove it if it's useless.
> 
> commit 8f716c9b5febf6ed0f5fedb7c9407cd0c25b2796
> Author: Tom Lendacky <thomas.lendacky@amd.com>
> Date:   Mon Jul 17 16:10:16 2017 -0500
> 
>     x86/mm: Add support to access boot related data in the clear
> 
> Hi Tom,
> 
> Can you help check and tell your intention why the argument 'size' is
> added into early_memremap_is_setup_data() and memremap_is_setup_data().

That was a long time ago... I probably used it while I was developing the
support and then never removed it in the final version where it wasn't used.

Thanks,
Tom

> 
> Thanks
> Baoquan
> 
>>
>> ---
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 70b02fc61d93..e461d8e26871 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -632,8 +632,7 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
>>   * Examine the physical address to determine if it is boot data by checking
>>   * it against the boot params setup_data chain.
>>   */
>> -static bool memremap_is_setup_data(resource_size_t phys_addr,
>> -				   unsigned long size)
>> +static bool memremap_is_setup_data(resource_size_t phys_addr)
>>  {
>>  	struct setup_indirect *indirect;
>>  	struct setup_data *data;
>> @@ -769,7 +768,7 @@ bool arch_memremap_can_ram_remap(resource_size_t phys_addr, unsigned long size,
>>  		return false;
>>  
>>  	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
>> -		if (memremap_is_setup_data(phys_addr, size) ||
>> +		if (memremap_is_setup_data(phys_addr) ||
>>  		    memremap_is_efi_data(phys_addr, size))
>>  			return false;
>>  	}
>>
>> -- 
>> Regards/Gruss,
>>     Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
> 
> 

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

[Baoquan He]

On 10/30/24 at 07:49am, Tom Lendacky wrote:
> On 10/29/24 19:53, Baoquan He wrote:
> > On 10/29/24 at 07:11pm, Borislav Petkov wrote:
> >> On Wed, Sep 11, 2024 at 04:16:14PM +0800, 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,
> >>
> >> Huh?
> > 
> > Thanks for looking into this.
> > 
> > I ever doubted this, guess it could use the unused 'size' to avoid
> > warning? Noticed Tom introduced it at the beginning. It's better idea to
> > remove it if it's useless.
> > 
> > commit 8f716c9b5febf6ed0f5fedb7c9407cd0c25b2796
> > Author: Tom Lendacky <thomas.lendacky@amd.com>
> > Date:   Mon Jul 17 16:10:16 2017 -0500
> > 
> >     x86/mm: Add support to access boot related data in the clear
> > 
> > Hi Tom,
> > 
> > Can you help check and tell your intention why the argument 'size' is
> > added into early_memremap_is_setup_data() and memremap_is_setup_data().
> 
> That was a long time ago... I probably used it while I was developing the
> support and then never removed it in the final version where it wasn't used.

Thanks for confirming. Then we can remove it to avoid confusion.

Hi Boris,

Should I send the fixing patch alone and clean up the useless argument
'size' later, or squash them into one patch?

Thanks
Baoquan

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

[Borislav Petkov]

On Thu, Oct 31, 2024 at 11:41:12AM +0800, Baoquan He wrote:
> Should I send the fixing patch alone and clean up the useless argument
> 'size' later, or squash them into one patch?

First the fix, then the cleanup.

Btw, that fix wants to go to stable no? Seeing how it breaks certain machines
with IMA and kdump and SMe...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Baoquan He]

On 11/01/24 at 05:18pm, Borislav Petkov wrote:
> On Thu, Oct 31, 2024 at 11:41:12AM +0800, Baoquan He wrote:
> > Should I send the fixing patch alone and clean up the useless argument
> > 'size' later, or squash them into one patch?
> 
> First the fix, then the cleanup.

Sure, will do. Thanks a lot.

> 
> Btw, that fix wants to go to stable no? Seeing how it breaks certain machines
> with IMA and kdump and SMe...

Yeah, it should be added to stable. Distros may get both SME/IMA set not
as early as the bug introduced, while anyone doing so in an earlier kernel
will see the problem.

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

[Borislav Petkov]

On Sat, Nov 02, 2024 at 08:23:33AM +0800, Baoquan He wrote:
> Yeah, it should be added to stable. Distros may get both SME/IMA set not
> as early as the bug introduced, while anyone doing so in an earlier kernel
> will see the problem.

Ok, I'll take your 2/2 next week and you can then send the cleanup ontop.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

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.

Then, when you do the cleanup, do the following:

- 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

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

[Baoquan He]

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.

Just got a machine and building kernel, will report here when testing is
done.

> 
> Then, when you do the cleanup, do the following:
> 
> - 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... :-\

Will clean them all up as suggested. Thanks.

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

[Baoquan He]

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
> 

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

[Borislav Petkov]

On Wed, Nov 13, 2024 at 08:55:47PM +0800, Baoquan He wrote:
> 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.

Thanks, lemme queue it.

> Will post cleanup patch later. Thanks.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v3 2/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[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 a code bug in early_memremap_is_setup_data()
where checking the embedded content inside setup_data takes wrong range
calculation. 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.

In this case, 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>
Acked-by: Tom Lendacky <thomas.lendacky@amd.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

[tip: x86/urgent] x86/mm: Fix a kdump kernel failure on SME system when CONFIG_IMA_KEXEC=y

[tip-bot2 for Baoquan He]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     8d9ffb2fe65a6c4ef114e8d4f947958a12751bbe
Gitweb:        https://git.kernel.org/tip/8d9ffb2fe65a6c4ef114e8d4f947958a12751bbe
Author:        Baoquan He <bhe@redhat.com>
AuthorDate:    Wed, 11 Sep 2024 16:16:15 +08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Wed, 13 Nov 2024 14:11:33 +01:00

x86/mm: Fix a kdump kernel failure on SME system when CONFIG_IMA_KEXEC=y

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 70b02fc..8d29163 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;
 		}

Re: [PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Baoquan He]

Hi,

On 09/11/24 at 04:16pm, Baoquan He wrote:
> Currently, distros like Fedora/RHEL have enabled CONFIG_IMA_KEXEC by
> default. This makes kexec/kdump kernel always fail to boot up on SME
> platform because of a code bug. By debugging, the root cause is found
> out and bug is fixed with this patchset.

PING.

Can this be added into 6.12 so that SME system is available? Please
tell if there's any concern or comment.

Thanks
Baoquan

Re: [PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Baoquan He]

On 09/30/24 at 10:59am, Baoquan He wrote:
> Hi,
> 
> On 09/11/24 at 04:16pm, Baoquan He wrote:
> > Currently, distros like Fedora/RHEL have enabled CONFIG_IMA_KEXEC by
> > default. This makes kexec/kdump kernel always fail to boot up on SME
> > platform because of a code bug. By debugging, the root cause is found
> > out and bug is fixed with this patchset.
> 
> PING.
> 
> Can this be added into 6.12 so that SME system is available? Please
> tell if there's any concern or comment.

Ping again.

Re: [PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Andrew Morton]

On Wed, 11 Sep 2024 16:16:13 +0800 Baoquan He <bhe@redhat.com> wrote:

> Currently, distros like Fedora/RHEL have enabled CONFIG_IMA_KEXEC by
> default. This makes kexec/kdump kernel always fail to boot up on SME
> platform because of a code bug. By debugging, the root cause is found
> out and bug is fixed with this patchset.

[1/1] is a cleanup.  [2/2] fixes a bug which appears to go all the way
back to 5.10.  The bugfix patch has a dependency on the cleanup, which
is unfortunate.

We could add the Fixes: to [1/1] and add cc:stable to both patches so
they get backported into -stable kernels together.  But I think it's
nicer to just concentrate on the single bugfix patch (with Fixes: and
cc:stable) and do the cleanup later, in the usual fashion.

So can I suggest a resend please?

Re: [PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Baoquan He]

On 10/29/24 at 06:23pm, Andrew Morton wrote:
> On Wed, 11 Sep 2024 16:16:13 +0800 Baoquan He <bhe@redhat.com> wrote:
> 
> > Currently, distros like Fedora/RHEL have enabled CONFIG_IMA_KEXEC by
> > default. This makes kexec/kdump kernel always fail to boot up on SME
> > platform because of a code bug. By debugging, the root cause is found
> > out and bug is fixed with this patchset.
> 
> [1/1] is a cleanup.  [2/2] fixes a bug which appears to go all the way
> back to 5.10.  The bugfix patch has a dependency on the cleanup, which
> is unfortunate.
> 
> We could add the Fixes: to [1/1] and add cc:stable to both patches so
> they get backported into -stable kernels together.  But I think it's
> nicer to just concentrate on the single bugfix patch (with Fixes: and
> cc:stable) and do the cleanup later, in the usual fashion.

Totally agree, thanks a lot.

> 
> So can I suggest a resend please?

Will send a standalone patch to fix the bug, and send clean up patch
after the fix patch is settled.

Re: [PATCH v3 0/2] x86/mm/sme: fix the kdump kernel breakage on SME system when CONFIG_IMA_KEXEC=y

[Borislav Petkov]

On Tue, Oct 29, 2024 at 06:23:06PM -0700, Andrew Morton wrote:
> So can I suggest a resend please?

Andrew, you can ignore. It is on my radar now.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette