2020-02-02 17:17:54

by Arvind Sankar

[permalink] [raw]
Subject: [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes

This series fixes a potential bug in EFI mixed-mode and leaves GDT
handling to startup_{32,64} instead of efi_main.

The first patch removes KEEP_SEGMENTS support in loadflags, this is
unused now (details in patch 1 commit msg), to slightly simplify
subsequent changes.

The second patch fixes a potential bug in EFI mixed-mode, where we are
currently relying on the firmware GDT having a particular layout: a
CODE32 segment as descriptor 2 and a DATA segment as descriptor 3.

The third patch adds some safety during kernel decompression by updating
the GDTR to point to the copied GDT, rather than the old one which may
have been overwritten.

The fourth patch adds cld/cli to startup_64, and the fifth patch removes
all the GDT setup from efi_main and adds it to the 32-bit kernel's
startup_32. The 64-bit kernel already does GDT setup. This should be
safer as this code can keep track of where the .data section is moving
and ensure that GDTR is pointing to a clean copy of the GDT.

The last two patches are to fix an off-by-one in the GDT limit and do a
micro-optimization to the GDT loading instructions.

Changes from v1:
- added removal of KEEP_SEGMENTS
- added the mixed-mode fix
- completely removed GDT setup from efi_main, including for the 32-bit
kernel
- dropped documentation patches for now

Arvind Sankar (7):
x86/boot: Remove KEEP_SEGMENTS support
efi/x86: Don't depend on firmware GDT layout
x86/boot: Reload GDTR after copying to the end of the buffer
x86/boot: Clear direction and interrupt flags in startup_64
efi/x86: Remove GDT setup from efi_main
x86/boot: GDT limit value should be size - 1
x86/boot: Micro-optimize GDT loading instructions

Documentation/x86/boot.rst | 8 +-
arch/x86/boot/compressed/eboot.c | 103 ------------------------
arch/x86/boot/compressed/efi_thunk_64.S | 29 +++++--
arch/x86/boot/compressed/head_32.S | 48 +++++++----
arch/x86/boot/compressed/head_64.S | 66 ++++++++-------
arch/x86/kernel/head_32.S | 6 --
6 files changed, 99 insertions(+), 161 deletions(-)

--
2.24.1


2020-02-02 17:18:21

by Arvind Sankar

[permalink] [raw]
Subject: [PATCH v2 7/7] x86/boot: Micro-optimize GDT loading instructions

Rearrange the instructions a bit to use a 32-bit displacement once
instead of 2/3 times. This saves 8 bytes of machine code.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index c36e6156b6a3..a4f5561c1c0e 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -69,8 +69,9 @@ SYM_FUNC_START(startup_32)
subl $1b, %ebp

/* Load new GDT with the 64bit segments using 32bit descriptor */
- addl %ebp, gdt+2(%ebp)
- lgdt gdt(%ebp)
+ leal gdt(%ebp), %eax
+ movl %eax, 2(%eax)
+ lgdt (%eax)

/* Load segment registers with our descriptors */
movl $__BOOT_DS, %eax
@@ -355,9 +356,9 @@ SYM_CODE_START(startup_64)
*/

/* Make sure we have GDT with 32-bit code segment */
- leaq gdt(%rip), %rax
- movq %rax, gdt64+2(%rip)
- lgdt gdt64(%rip)
+ leaq gdt64(%rip), %rax
+ addq %rax, 2(%rax)
+ lgdt (%rax)

/*
* paging_prepare() sets up the trampoline and checks if we need to
@@ -625,12 +626,12 @@ SYM_FUNC_END(.Lno_longmode)
.data
SYM_DATA_START_LOCAL(gdt64)
.word gdt_end - gdt - 1
- .quad 0
+ .quad gdt - gdt64
SYM_DATA_END(gdt64)
.balign 8
SYM_DATA_START_LOCAL(gdt)
.word gdt_end - gdt - 1
- .long gdt
+ .long 0
.word 0
.quad 0x00cf9a000000ffff /* __KERNEL32_CS */
.quad 0x00af9a000000ffff /* __KERNEL_CS */
--
2.24.1

2020-02-02 18:04:34

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes

On Sun, 2 Feb 2020 at 18:13, Arvind Sankar <[email protected]> wrote:
>
> This series fixes a potential bug in EFI mixed-mode and leaves GDT
> handling to startup_{32,64} instead of efi_main.
>
> The first patch removes KEEP_SEGMENTS support in loadflags, this is
> unused now (details in patch 1 commit msg), to slightly simplify
> subsequent changes.
>
> The second patch fixes a potential bug in EFI mixed-mode, where we are
> currently relying on the firmware GDT having a particular layout: a
> CODE32 segment as descriptor 2 and a DATA segment as descriptor 3.
>
> The third patch adds some safety during kernel decompression by updating
> the GDTR to point to the copied GDT, rather than the old one which may
> have been overwritten.
>
> The fourth patch adds cld/cli to startup_64, and the fifth patch removes
> all the GDT setup from efi_main and adds it to the 32-bit kernel's
> startup_32. The 64-bit kernel already does GDT setup. This should be
> safer as this code can keep track of where the .data section is moving
> and ensure that GDTR is pointing to a clean copy of the GDT.
>
> The last two patches are to fix an off-by-one in the GDT limit and do a
> micro-optimization to the GDT loading instructions.
>

Thanks Arvind.

This looks good to me,

Acked-by: Ard Biesheuvel <[email protected]>

but I'm a bit out of my depth here when it comes to x86'ology so it's
really up to the x86 maintainers.


> Changes from v1:
> - added removal of KEEP_SEGMENTS
> - added the mixed-mode fix
> - completely removed GDT setup from efi_main, including for the 32-bit
> kernel
> - dropped documentation patches for now
>
> Arvind Sankar (7):
> x86/boot: Remove KEEP_SEGMENTS support
> efi/x86: Don't depend on firmware GDT layout
> x86/boot: Reload GDTR after copying to the end of the buffer
> x86/boot: Clear direction and interrupt flags in startup_64
> efi/x86: Remove GDT setup from efi_main
> x86/boot: GDT limit value should be size - 1
> x86/boot: Micro-optimize GDT loading instructions
>
> Documentation/x86/boot.rst | 8 +-
> arch/x86/boot/compressed/eboot.c | 103 ------------------------
> arch/x86/boot/compressed/efi_thunk_64.S | 29 +++++--
> arch/x86/boot/compressed/head_32.S | 48 +++++++----
> arch/x86/boot/compressed/head_64.S | 66 ++++++++-------
> arch/x86/kernel/head_32.S | 6 --
> 6 files changed, 99 insertions(+), 161 deletions(-)
>
> --
> 2.24.1
>