Message-ID: <Z8qXVLiab5L-XkgP@kernel.org>
Date: Fri, 7 Mar 2025 08:51:00 +0200
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Ross Philipson <ross.philipson@...cle.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
	linux-integrity@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-crypto@...r.kernel.org, kexec@...ts.infradead.org,
	linux-efi@...r.kernel.org, iommu@...ts.linux-foundation.org,
	dpsmith@...rtussolutions.com, tglx@...utronix.de, mingo@...hat.com,
	bp@...en8.de, hpa@...or.com, dave.hansen@...ux.intel.com,
	ardb@...nel.org, mjg59@...f.ucam.org,
	James.Bottomley@...senpartnership.com, peterhuewe@....de,
	jgg@...pe.ca, luto@...capital.net, nivedita@...m.mit.edu,
	herbert@...dor.apana.org.au, davem@...emloft.net, corbet@....net,
	ebiederm@...ssion.com, dwmw2@...radead.org,
	baolu.lu@...ux.intel.com, kanth.ghatraju@...cle.com,
	andrew.cooper3@...rix.com, trenchboot-devel@...glegroups.com
Subject: Re: [PATCH v12 08/19] x86/boot: Place TXT MLE header in the
 kernel_info section

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@...cle.com>
> Suggested-by: Ard Biesheuvel <ardb@...nel.org>
> Reviewed-by: Ard Biesheuvel <ardb@...nel.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.

> +#endif
> -- 
> 2.39.3
> 

BR, Jarkko

Powered by blists - more mailing lists