| Message ID | 20220921145422.437618-3-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | x86: head_64.S spring cleaning | expand |
On Wed, Sep 21, 2022 at 04:54:08PM +0200, Ard Biesheuvel wrote: > Move the code that stores the arguments passed to the EFI entrypoint > into the .text section, so that it can be moved into a separate > compilation unit in a subsequent patch. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/boot/compressed/head_64.S | 34 ++++++++++++-------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > index d33f060900d2..1ba2fc2357e6 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -303,24 +303,28 @@ SYM_FUNC_START(efi32_stub_entry) > popl %ecx > popl %edx > popl %esi > + jmp efi32_entry > +SYM_FUNC_END(efi32_stub_entry) > > + .text > +SYM_FUNC_START_LOCAL(efi32_entry) > call 1f > -1: pop %ebp > - subl $ rva(1b), %ebp > - > - movl %esi, rva(efi32_boot_args+8)(%ebp) > -SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) > - movl %ecx, rva(efi32_boot_args)(%ebp) > - movl %edx, rva(efi32_boot_args+4)(%ebp) > - movb $0, rva(efi_is64)(%ebp) > +1: pop %ebx I'm guessing according to the EFI mixed mode calling convention, %ebx is not a live register which gets overwritten here...? Looking at efi32_pe_entry() from where this is called, %ebx looks live. What am I missing?
On Thu, 6 Oct 2022 at 12:42, Borislav Petkov <bp@alien8.de> wrote: > > On Wed, Sep 21, 2022 at 04:54:08PM +0200, Ard Biesheuvel wrote: > > Move the code that stores the arguments passed to the EFI entrypoint > > into the .text section, so that it can be moved into a separate > > compilation unit in a subsequent patch. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/x86/boot/compressed/head_64.S | 34 ++++++++++++-------- > > 1 file changed, 20 insertions(+), 14 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > > index d33f060900d2..1ba2fc2357e6 100644 > > --- a/arch/x86/boot/compressed/head_64.S > > +++ b/arch/x86/boot/compressed/head_64.S > > @@ -303,24 +303,28 @@ SYM_FUNC_START(efi32_stub_entry) > > popl %ecx > > popl %edx > > popl %esi > > + jmp efi32_entry > > +SYM_FUNC_END(efi32_stub_entry) > > > > + .text > > +SYM_FUNC_START_LOCAL(efi32_entry) > > call 1f > > -1: pop %ebp > > - subl $ rva(1b), %ebp > > - > > - movl %esi, rva(efi32_boot_args+8)(%ebp) > > -SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) > > - movl %ecx, rva(efi32_boot_args)(%ebp) > > - movl %edx, rva(efi32_boot_args+4)(%ebp) > > - movb $0, rva(efi_is64)(%ebp) > > +1: pop %ebx > > I'm guessing according to the EFI mixed mode calling convention, %ebx is > not a live register which gets overwritten here...? > > Looking at efi32_pe_entry() from where this is called, %ebx looks live. > > What am I missing? > efi32_pe_entry() preserves and restores the caller's value of %ebx, because from there, we might actually return control to the firmware. The value it keeps in %ebx itself is not live when it jumps to efi32_entry - it stores its value into image_offset, which is reloaded from memory at a later point. efi32_stub_entry() is the 'EFI handover protocol' entry point, which cannot return to the firmware (and we discard the return address already) so %ebx can be clobbered.
On Thu, Oct 06, 2022 at 12:56:09PM +0200, Ard Biesheuvel wrote: > efi32_pe_entry() preserves and restores the caller's value of %ebx, > because from there, we might actually return control to the firmware. > The value it keeps in %ebx itself is not live when it jumps to > efi32_entry - it stores its value into image_offset, which is reloaded > from memory at a later point. Hmm, might be prudent to have a comment there because it is using %ebx a couple of insns before the JMP: subl %esi, %ebx ^^^^ movl %ebx, rva(image_offset)(%ebp) // save image_offset <--- I think you mean that after this, %ebx is not needed anymore? xorl %esi, %esi jmp efi32_entry 2: popl %edi // restore callee-save registers popl %ebx and this restores its original value ofc. > efi32_stub_entry() is the 'EFI handover protocol' entry point, which > cannot return to the firmware (and we discard the return address > already) so %ebx can be clobbered. That info would be good to have in a comment above it. Thx.
On Thu, 6 Oct 2022 at 13:13, Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Oct 06, 2022 at 12:56:09PM +0200, Ard Biesheuvel wrote: > > efi32_pe_entry() preserves and restores the caller's value of %ebx, > > because from there, we might actually return control to the firmware. > > The value it keeps in %ebx itself is not live when it jumps to > > efi32_entry - it stores its value into image_offset, which is reloaded > > from memory at a later point. > > Hmm, might be prudent to have a comment there because it is using %ebx a > couple of insns before the JMP: > > subl %esi, %ebx > ^^^^ > movl %ebx, rva(image_offset)(%ebp) // save image_offset > > <--- I think you mean that after this, %ebx is not needed anymore? > Exactly. > xorl %esi, %esi > jmp efi32_entry > > 2: popl %edi // restore callee-save registers > popl %ebx > > and this restores its original value ofc. > > > efi32_stub_entry() is the 'EFI handover protocol' entry point, which > > cannot return to the firmware (and we discard the return address > > already) so %ebx can be clobbered. > > That info would be good to have in a comment above it. > Fair enough.
On Thu, 6 Oct 2022 at 13:19, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 6 Oct 2022 at 13:13, Borislav Petkov <bp@alien8.de> wrote: > > > > On Thu, Oct 06, 2022 at 12:56:09PM +0200, Ard Biesheuvel wrote: > > > efi32_pe_entry() preserves and restores the caller's value of %ebx, > > > because from there, we might actually return control to the firmware. > > > The value it keeps in %ebx itself is not live when it jumps to > > > efi32_entry - it stores its value into image_offset, which is reloaded > > > from memory at a later point. > > > > Hmm, might be prudent to have a comment there because it is using %ebx a > > couple of insns before the JMP: > > > > subl %esi, %ebx > > ^^^^ > > movl %ebx, rva(image_offset)(%ebp) // save image_offset > > > > <--- I think you mean that after this, %ebx is not needed anymore? > > > > Exactly. > > > xorl %esi, %esi > > jmp efi32_entry > > > > 2: popl %edi // restore callee-save registers > > popl %ebx > > > > and this restores its original value ofc. > > > > > efi32_stub_entry() is the 'EFI handover protocol' entry point, which > > > cannot return to the firmware (and we discard the return address > > > already) so %ebx can be clobbered. > > > > That info would be good to have in a comment above it. > > > > Fair enough. I'll add the below in the next revision --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -307,6 +307,19 @@ SYM_FUNC_START(efi32_stub_entry) SYM_FUNC_END(efi32_stub_entry) .text +/* + * This is the common EFI stub entry point for mixed mode. + * + * Arguments: %ecx image handle + * %edx EFI system table pointer + * %esi struct bootparams pointer (or NULL when not using + * the EFI handover protocol) + * + * Since this is the point of no return for ordinary execution, no registers + * are considered live except for the function parameters. [Note that the EFI + * stub may still exit and return to the firmware using the Exit() EFI boot + * service.] + */ SYM_FUNC_START_LOCAL(efi32_entry) call 1f 1: pop %ebx @@ -837,7 +850,8 @@ SYM_FUNC_START(efi32_pe_entry) subl %esi, %ebx movl %ebx, rva(image_offset)(%ebp) // save image_offset xorl %esi, %esi - jmp efi32_entry + jmp efi32_entry // pass %ecx, %edx, %esi + // no other registers remain live 2: popl %edi // restore callee-save registers popl %ebx
On Thu, Oct 06, 2022 at 02:27:54PM +0200, Ard Biesheuvel wrote:
> I'll add the below in the next revision
LGTM.
Thx.
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index d33f060900d2..1ba2fc2357e6 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -303,24 +303,28 @@ SYM_FUNC_START(efi32_stub_entry) popl %ecx popl %edx popl %esi + jmp efi32_entry +SYM_FUNC_END(efi32_stub_entry) + .text +SYM_FUNC_START_LOCAL(efi32_entry) call 1f -1: pop %ebp - subl $ rva(1b), %ebp - - movl %esi, rva(efi32_boot_args+8)(%ebp) -SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) - movl %ecx, rva(efi32_boot_args)(%ebp) - movl %edx, rva(efi32_boot_args+4)(%ebp) - movb $0, rva(efi_is64)(%ebp) +1: pop %ebx /* Save firmware GDTR and code/data selectors */ - sgdtl rva(efi32_boot_gdt)(%ebp) - movw %cs, rva(efi32_boot_cs)(%ebp) - movw %ds, rva(efi32_boot_ds)(%ebp) + sgdtl (efi32_boot_gdt - 1b)(%ebx) + movw %cs, (efi32_boot_cs - 1b)(%ebx) + movw %ds, (efi32_boot_ds - 1b)(%ebx) /* Store firmware IDT descriptor */ - sidtl rva(efi32_boot_idt)(%ebp) + sidtl (efi32_boot_idt - 1b)(%ebx) + + /* Store boot arguments */ + leal (efi32_boot_args - 1b)(%ebx), %ebx + movl %ecx, 0(%ebx) + movl %edx, 4(%ebx) + movl %esi, 8(%ebx) + movb $0x0, 12(%ebx) // efi_is64 /* Disable paging */ movl %cr0, %eax @@ -328,7 +332,8 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) movl %eax, %cr0 jmp startup_32 -SYM_FUNC_END(efi32_stub_entry) +SYM_FUNC_END(efi32_entry) + __HEAD #endif .code64 @@ -831,7 +836,8 @@ SYM_FUNC_START(efi32_pe_entry) */ subl %esi, %ebx movl %ebx, rva(image_offset)(%ebp) // save image_offset - jmp efi32_pe_stub_entry + xorl %esi, %esi + jmp efi32_entry 2: popl %edi // restore callee-save registers popl %ebx
Move the code that stores the arguments passed to the EFI entrypoint into the .text section, so that it can be moved into a separate compilation unit in a subsequent patch. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/x86/boot/compressed/head_64.S | 34 ++++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-)