Re: [PATCH v6 08/14] x86: Secure Launch kernel late boot stub

[Ross Philipson]

On 5/12/23 11:44, Thomas Gleixner wrote:
> On Thu, May 04 2023 at 14:50, Ross Philipson wrote:
> > The routine slaunch_setup is called out of the x86 specific setup_arch
>
> Can you please make functions visible in changelogs by appending (),
> i.e. setup_arch() ?

Yes I will.

> See https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/maintainer-tip.html__;!!ACWV5N9M2RV99hQ!IpJMDBpAvJRDAh0tZI_nMv0zZqwQDnxFjBEKRitYq4JU-iV-NnXg28lGtTwb1ynVA4XEy5n9aSdIekxkztyZ$
> for further hints.
>
> > +static u32 sl_flags;
> > +static struct sl_ap_wake_info ap_wake_info;
> > +static u64 evtlog_addr;
> > +static u32 evtlog_size;
> > +static u64 vtd_pmr_lo_size;
>
> Is any of this modifyable after boot? If not then this wants to be
> annotated with __ro_after_init.

I believe you are correct and these are never modified after boot so I 
will do this.

> > +/* This should be plenty of room */
> > +static u8 txt_dmar[PAGE_SIZE] __aligned(16);
> > +
> > +u32 slaunch_get_flags(void)
> > +{
> > +	return sl_flags;
> > +}
> > +EXPORT_SYMBOL(slaunch_get_flags);
>
> What needs this export? If there is a reason then please EXPORT_SYMBOL_GPL()

I think that may be incorrect. I will look into it.

> > +struct sl_ap_wake_info *slaunch_get_ap_wake_info(void)
> > +{
> > +	return &ap_wake_info;
> > +}
>
> If you return a pointer, then there is not much of point for encapsulating.

I am sorry, I am not 100% sure what you mean.

> > +struct acpi_table_header *slaunch_get_dmar_table(struct acpi_table_header *dmar)
>
> Some explanation on public visible functions would be really useful.

I can add that.

> > +{
> > +	/* The DMAR is only stashed and provided via TXT on Intel systems */
>
> -ENOPARSE.

I take it you mean you don't understand the comment. I will try to make 
it clearer.

> > +	if (memcmp(txt_dmar, "DMAR", 4))
> > +		return dmar;
> > +
> > +	return (struct acpi_table_header *)(&txt_dmar[0]);
>
> s/&txt_dmar[0]/txt_dmar/ No?

Just an old habit. I can change it.

> > +}
>
> > +void __noreturn slaunch_txt_reset(void __iomem *txt,
> > +				  const char *msg, u64 error)
>
> Please avoid these line breaks. We lifted the 80 character limit quite
> some time ago.

Ack

> > +
> > +	/* Iterate over heap tables looking for table of "type" */
> > +	for (i = 0; i < type; i++) {
> > +		base += offset;
> > +		heap = early_memremap(base, sizeof(u64));
> > +		if (!heap)
> > +			slaunch_txt_reset(txt,
> > +				"Error early_memremap of heap for heap walk\n",
> > +				SL_ERROR_HEAP_MAP);
>
> This is horrible to read.
>
> 		if (!heap) {
> 			slaunch_txt_reset(txt, "Error early_memremap of heap for heap walk\n",
>                          		  SL_ERROR_HEAP_MAP);
>                  }
>
> See documentation about bracket rules.

Will do.

> > +
> > +/*
> > + * TXT stashes a safe copy of the DMAR ACPI table to prevent tampering.
> > + * It is stored in the TXT heap. Fetch it from there and make it available
> > + * to the IOMMU driver.
> > + */
> > +static void __init slaunch_copy_dmar_table(void __iomem *txt)
> > +{
> > +	struct txt_sinit_mle_data *sinit_mle_data;
> > +	u32 field_offset, dmar_size, dmar_offset;
> > +	void *dmar;
> > +
> > +	memset(&txt_dmar, 0, PAGE_SIZE);
>
> txt_dmar is statically allocated so it's already zero, no?

Yes. This may be left over from an older iteration of the patches. I 
will ditch it.

> > +/*
> > + * Intel TXT specific late stub setup and validation.
> > + */
> > +void __init slaunch_setup_txt(void)
> > +{
> > +	u64 one = TXT_REGVALUE_ONE, val;
> > +	void __iomem *txt;
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SMX))
> > +		return;
> > +
> > +	/*
> > +	 * If booted through secure launch entry point, the loadflags
> > +	 * option will be set.
> > +	 */
> > +	if (!(boot_params.hdr.loadflags & SLAUNCH_FLAG))
> > +		return;
> > +
> > +	/*
> > +	 * See if SENTER was done by reading the status register in the
> > +	 * public space. If the public register space cannot be read, TXT may
> > +	 * be disabled.
> > +	 */
> > +	txt = early_ioremap(TXT_PUB_CONFIG_REGS_BASE,
> > +			    TXT_NR_CONFIG_PAGES * PAGE_SIZE);
> > +	if (!txt)
> > +		return;
>
> Wait. You have established above that SMX is available and the boot has
> set the SLAUNCH flag.
>
> So if that ioremap() fails then there is an issue with the fixmaps.
>
> How is returning here sensible? The system will just die later on in the
> worst case with some undecodable issue.

Good point. I don't think I can do a TXT reset at this point but I could 
panic.

Thanks for the review,
Ross

> Thanks,
>
>          tglx