Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub

From: Ross Philipson <ross.philipson@oracle.com>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	iommu@lists.linux-foundation.org,
	linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org,
	dpsmith@apertussolutions.com, tglx@linutronix.de,
	mingo@redhat.com, bp@alien8.de, hpa@zytor.com,
	luto@amacapital.net, trenchboot-devel@googlegroups.com
Subject: Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub
Date: Tue, 29 Sep 2020 10:03:47 -0400	[thread overview]
Message-ID: <d34c189c-4528-0458-0b84-cfd36dc068b3@oracle.com> (raw)
In-Reply-To: <20200925191842.GA643740@rani.riverdale.lan>

On 9/25/20 3:18 PM, Arvind Sankar wrote:
> On Fri, Sep 25, 2020 at 10:56:43AM -0400, Ross Philipson wrote:
>> On 9/24/20 1:38 PM, Arvind Sankar wrote:
>>> On Thu, Sep 24, 2020 at 10:58:35AM -0400, Ross Philipson wrote:
>>>
>>>> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
>>>> index 97d37f0..42043bf 100644
>>>> --- a/arch/x86/boot/compressed/head_64.S
>>>> +++ b/arch/x86/boot/compressed/head_64.S
>>>> @@ -279,6 +279,21 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
>>>>  SYM_FUNC_END(efi32_stub_entry)
>>>>  #endif
>>>>  
>>>> +#ifdef CONFIG_SECURE_LAUNCH
>>>> +SYM_FUNC_START(sl_stub_entry)
>>>> +	/*
>>>> +	 * On entry, %ebx has the entry abs offset to sl_stub_entry. To
>>>> +	 * find the beginning of where we are loaded, sub off from the
>>>> +	 * beginning.
>>>> +	 */
>>>
>>> This requirement should be added to the documentation. Is it necessary
>>> or can this stub just figure out the address the same way as the other
>>> 32-bit entry points, using the scratch space in bootparams as a little
>>> stack?
>>
>> It is based on the state of the BSP when TXT vectors to the measured
>> launch environment. It is documented in the TXT spec and the SDMs.
>>
> 
> I think it would be useful to add to the x86 boot documentation how
> exactly this new entry point is called, even if it's just adding a link
> to some section of those specs. The doc should also say that an
> mle_header_offset of 0 means the kernel isn't secure launch enabled.

Ok will do.

> 
>>>
>>> For the 32-bit assembler code that's being added, tip/master now has
>>> changes that prevent the compressed kernel from having any runtime
>>> relocations.  You'll need to revise some of the code and the data
>>> structures initial values to avoid creating relocations.
>>
>> Could you elaborate on this some more? I am not sure I see places in the
>> secure launch asm that would be creating relocations like this.
>>
>> Thank you,
>> Ross
>>
> 
> You should see them if you do
> 	readelf -r arch/x86/boot/compressed/vmlinux
> 
> In terms of the code, things like:
> 
> 	addl    %ebx, (sl_gdt_desc + 2)(%ebx)
> 
> will create a relocation, because the linker interprets this as wanting
> the runtime address of sl_gdt_desc, rather than just the offset from
> startup_32.
> 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/arch/x86/boot/compressed/head_64.S*n48__;Iw!!GqivPVa7Brio!JpZWv1cCPZdjD2jbCCGT7P9UIVl_lhX7YjckAnUcvi927jwZI7X3nX0MpIAZOyktJds$ 
> 
> has a comment with some explanation and a macro that the 32-bit code in
> startup_32 uses to avoid creating relocations.
> 
> Since the SL code is in a different assembler file (and a different
> section), you can't directly use the same macro. I would suggest getting
> rid of sl_stub_entry and entering directly at sl_stub, and then the code
> in sl_stub.S can use sl_stub for the base address, defining the rva()
> macro there as
> 
> 	#define rva(X) ((X) - sl_stub)
> 
> You will also need to avoid initializing data with symbol addresses.
> 
> 	.long mle_header
> 	.long sl_stub_entry
> 	.long sl_gdt
> 
> will create relocations. The third one is easy, just replace it with
> sl_gdt - sl_gdt_desc and initialize it at runtime with
> 
> 	leal	rva(sl_gdt_desc)(%ebx), %eax
> 	addl	%eax, 2(%eax)
> 	lgdt	(%eax)
> 
> The other two are more messy, unfortunately there is no easy way to tell
> the linker what we want here. The other entry point addresses (for the
> EFI stub) are populated in a post-processing step after the compressed
> kernel has been linked, we could teach it to also update kernel_info.
> 
> Without that, for kernel_info, you could change it to store the offset
> of the MLE header from kernel_info, instead of from the start of the
> image.
> 
> For the MLE header, it could be moved to .head.text in head_64.S, and
> initialized with
> 	.long rva(sl_stub)
> This will also let it be placed at a fixed offset from startup_32, so
> that kernel_info can just be populated with a constant.

Thank you for the detailed reply. I am going to start digging into this now.

Ross

> 