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

[Tom Lendacky]

On 8/27/24 08:52, Tom Lendacky wrote:
> On 8/26/24 22:19, Baoquan He wrote:
>> On 08/26/24 at 09:24am, Tom Lendacky wrote:
>>> On 8/25/24 21:44, 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.
>>>>
>>>>
>>>> Here fix the code bug to make kexec/kdump kernel boot up successfully.
>>>>
>>>> Fixes: 8f716c9b5feb ("x86/mm: Add support to access boot related data in the clear")
>>>
>>> The check that was modified was added by:
>>> 	b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
>>>
>>> The SETUP_INDIRECT patches seem to be the issue here.
>>
>> Hmm, I didn't check it carefully, thanks for addding this info. While
>> after checking commit b3c72fc9a78e, I feel the adding code was trying to
>> fix your original early_memremap_is_setup_data(). Even though
>> SETUP_INDIRECT type of setup_data has been added, the original
>> early_memremap_is_setup_data() only check the starting address and
>> the content of struct setup_data, that's obviously wrong.
> 
> IIRC, when this function was created, the value of "len" in setup_data
> included the length of "data", so the calculation was correct. Everything
> was contiguous in a setup_data element.
> 
>>
>> arch/x86/include/uapi/asm/setup_data.h:
>> /* extensible setup data list node */
>> struct setup_data {
>>         __u64 next;
>>         __u32 type;
>>         __u32 len;
>>         __u8 data[];
>> };
>>
>> As you can see, the zero-length will embed the carried data which is
>> actually expected and adjacent to its carrier, the struct setup_data.
> 
> Right, and "len" is the length of that data. So paddr + len goes to the
> end of the overall setup_data.

Ah, I see what you're saying. "len" doesn't include the size of the
setup_data structure, only the data. If so, then, yes, adding a sizeof()
to the calculation in the if statement is correct.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>>>