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

[Baoquan He <bhe@redhat.com>]

On 08/27/24 at 09:00am, Tom Lendacky wrote:
> 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.

Exactly. That could confuse people sometime.