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. > > > > 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://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/arch/x86/boot/compressed/head_64.S#n48 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.
