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

[Ross Philipson <ross.philipson@oracle.com>]

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec