Re: [PATCH v12 08/19] x86/boot: Place TXT MLE header in the kernel_info section

[ross.philipson@oracle.com]

On 3/6/25 10:51 PM, Jarkko Sakkinen wrote:
> On Thu, Dec 19, 2024 at 11:42:05AM -0800, Ross Philipson wrote:
>> The MLE (measured launch environment) header must be locatable by the
>> boot loader and TXT must be setup to do a launch with this header's
> 
> (cutting the hairs) nit: /TXT/Intel TXT/
> 
>> location. While the offset to the kernel_info structure does not need
>> to be at a fixed offset, the offsets in the header must be relative
>> offsets from the start of the setup kernel. The support in the linker
>> file achieves this.
> 
> This is too obfuscated and also sort of misses the action taken by
> the patch.
> 
> I presume the goal here is to add relative offset to the MLE header?
> Please state that explicitly.
> 
> Like for any possible kernel patch:
> 
> 1. Come out clean 110% transparent.
> 2. Full exposure what you're doing.
> 
> ;-)
> 
> That's the fastest possible path to actual results.
> 
>>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   arch/x86/boot/compressed/kernel_info.S | 50 +++++++++++++++++++++++---
>>   arch/x86/boot/compressed/vmlinux.lds.S |  7 ++++
>>   2 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/kernel_info.S b/arch/x86/boot/compressed/kernel_info.S
>> index f818ee8fba38..a0604a0d1756 100644
>> --- a/arch/x86/boot/compressed/kernel_info.S
>> +++ b/arch/x86/boot/compressed/kernel_info.S
>> @@ -1,12 +1,20 @@
>>   /* SPDX-License-Identifier: GPL-2.0 */
>>   
>> +#include <linux/linkage.h>
>>   #include <asm/bootparam.h>
>>   
>> -	.section ".rodata.kernel_info", "a"
>> +/*
>> + * The kernel_info structure is not placed at a fixed offest in the
>> + * kernel image. So this macro and the support in the linker file
>> + * allow the relative offsets for the MLE header within the kernel
>> + * image to be configured at build time.
>> + */
>> +#define roffset(X) ((X) - kernel_info)
>>   
>> -	.global kernel_info
>> +	.section ".rodata.kernel_info", "a"
>>   
>> -kernel_info:
>> +	.balign	16
>> +SYM_DATA_START(kernel_info)
>>   	/* Header, Linux top (structure). */
>>   	.ascii	"LToP"
>>   	/* Size. */
>> @@ -17,6 +25,40 @@ kernel_info:
>>   	/* Maximal allowed type for setup_data and setup_indirect structs. */
>>   	.long	SETUP_TYPE_MAX
>>   
>> +	/* Offset to the MLE header structure */
>> +#if IS_ENABLED(CONFIG_SECURE_LAUNCH)
>> +	.long	roffset(mle_header_offset)
>> +#else
>> +	.long	0
>> +#endif
>> +
>>   kernel_info_var_len_data:
>>   	/* Empty for time being... */
>> -kernel_info_end:
>> +SYM_DATA_END_LABEL(kernel_info, SYM_L_LOCAL, kernel_info_end)
>> +
>> +#if IS_ENABLED(CONFIG_SECURE_LAUNCH)
>> +	/*
>> +	 * The MLE Header per the TXT Specification, section 2.1
>> +	 * MLE capabilities, see table 4. Capabilities set:
>> +	 * bit 0: Support for GETSEC[WAKEUP] for RLP wakeup
>> +	 * bit 1: Support for RLP wakeup using MONITOR address
>> +	 * bit 2: The ECX register will contain the pointer to the MLE page table
>> +	 * bit 5: TPM 1.2 family: Details/authorities PCR usage support
>> +	 * bit 9: Supported format of TPM 2.0 event log - TCG compliant
>> +	 */
>> +SYM_DATA_START(mle_header)
>> +	.long	0x9082ac5a  /* UUID0 */
>> +	.long	0x74a7476f  /* UUID1 */
>> +	.long	0xa2555c0f  /* UUID2 */
>> +	.long	0x42b651cb  /* UUID3 */
>> +	.long	0x00000034  /* MLE header size */
>> +	.long	0x00020002  /* MLE version 2.2 */
>> +	.long	roffset(sl_stub_entry_offset) /* Linear entry point of MLE (virt. address) */
>> +	.long	0x00000000  /* First valid page of MLE */
>> +	.long	0x00000000  /* Offset within binary of first byte of MLE */
>> +	.long	roffset(_edata_offset) /* Offset within binary of last byte + 1 of MLE */
>> +	.long	0x00000227  /* Bit vector of MLE-supported capabilities */
>> +	.long	0x00000000  /* Starting linear address of command line (unused) */
>> +	.long	0x00000000  /* Ending linear address of command line (unused) */
> 
> Nit: I'd consider aligning these to few tab offsets after even tho it
> might cause checkpatch complain (which is fine when there are legitimite
> reasons to do so).
> 
> Would be easier to read.
> 
>> +SYM_DATA_END(mle_header)
>> +#endif
>> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
>> index 083ec6d7722a..f82184801462 100644
>> --- a/arch/x86/boot/compressed/vmlinux.lds.S
>> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
>> @@ -118,3 +118,10 @@ SECTIONS
>>   	}
>>   	ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
>>   }
>> +
>> +#ifdef CONFIG_SECURE_LAUNCH
>> +PROVIDE(kernel_info_offset      = ABSOLUTE(kernel_info - startup_32));
>> +PROVIDE(mle_header_offset       = kernel_info_offset + ABSOLUTE(mle_header - startup_32));
>> +PROVIDE(sl_stub_entry_offset    = kernel_info_offset + ABSOLUTE(sl_stub_entry - startup_32));
>> +PROVIDE(_edata_offset           = kernel_info_offset + ABSOLUTE(_edata - startup_32));
> 
> I'd enumerate these one by one in the commit message. I.e. what is added
> explicitly.

Everything you say here makes sense, we can incorporate these changes.

Thanks
Ross

> 
>> +#endif
>> -- 
>> 2.39.3
>>
> 
> BR, Jarkko