* [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes @ 2020-01-30 20:04 Arvind Sankar 2020-01-30 20:04 ` [PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly Arvind Sankar ` (9 more replies) 0 siblings, 10 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc For the 64-bit kernel, we do not need to setup a GDT in efi_main, as the next step in the boot, startup_64, will set one up. This series factors out the GDT setup into a separate function and restricts it to the 32-bit kernel. The memory allocation for the GDT is also changed from allocating a full page via efi_alloc_low to the required space (32 bytes) from alloc_pool boot service. The final two patches are doc fixes to clarify that the 32-bit EFI handover entry point also requires segments/GDT to be setup, and that for 64-bit mode we don't have any special segment requirements (the 64-bit doc currently is just a copy of the 32-bit doc which isn't right). Arvind Sankar (8): efi/x86: Use C wrapper instead of inline assembly efi/x86: Allocate the GDT pointer on the stack efi/x86: Factor GDT setup code into a function efi/x86: Only setup the GDT for 32-bit kernel efi/x86: Allocate only the required 32 bytes for the GDT efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} Documentation/x86/boot: Clarify segment requirements for EFI handover Documentation/x86/boot: Correct segment requirements for 64-bit boot Documentation/x86/boot.rst | 15 +- arch/x86/boot/compressed/eboot.c | 174 ++++++++++-------------- arch/x86/boot/compressed/efi_thunk_64.S | 4 +- 3 files changed, 85 insertions(+), 108 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 2/8] efi/x86: Allocate the GDT pointer on the stack Arvind Sankar ` (8 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc Call the C wrappers instead of inline assembly for cli and lgdt instructions. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 287393d725f0..f89caae60057 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -9,6 +9,7 @@ #pragma GCC visibility push(hidden) #include <linux/efi.h> +#include <linux/irqflags.h> #include <linux/pci.h> #include <asm/efi.h> @@ -877,8 +878,8 @@ struct boot_params *efi_main(efi_handle_t handle, desc++; } - asm volatile("cli"); - asm volatile ("lgdt %0" : : "m" (*gdt)); + raw_local_irq_disable(); + native_load_gdt(gdt); return boot_params; fail: -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/8] efi/x86: Allocate the GDT pointer on the stack 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar 2020-01-30 20:04 ` [PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 3/8] efi/x86: Factor GDT setup code into a function Arvind Sankar ` (7 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc The GDT pointer isn't needed after loading it into GDTR, so there is no need to dynamically allocate it. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index f89caae60057..a0a2fd0528af 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -713,7 +713,7 @@ struct boot_params *efi_main(efi_handle_t handle, efi_system_table_t *sys_table_arg, struct boot_params *boot_params) { - struct desc_ptr *gdt = NULL; + struct desc_ptr gdt; struct setup_header *hdr = &boot_params->hdr; efi_status_t status; struct desc_struct *desc; @@ -754,15 +754,8 @@ struct boot_params *efi_main(efi_handle_t handle, setup_quirks(boot_params); - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*gdt), - (void **)&gdt); - if (status != EFI_SUCCESS) { - efi_printk("Failed to allocate memory for 'gdt' structure\n"); - goto fail; - } - - gdt->size = 0x800; - status = efi_low_alloc(gdt->size, 8, (unsigned long *)&gdt->address); + gdt.size = 0x800; + status = efi_low_alloc(gdt.size, 8, (unsigned long *)&gdt.address); if (status != EFI_SUCCESS) { efi_printk("Failed to allocate memory for 'gdt'\n"); goto fail; @@ -794,8 +787,8 @@ struct boot_params *efi_main(efi_handle_t handle, goto fail; } - memset((char *)gdt->address, 0x0, gdt->size); - desc = (struct desc_struct *)gdt->address; + memset((char *)gdt.address, 0x0, gdt.size); + desc = (struct desc_struct *)gdt.address; /* The first GDT is a dummy. */ desc++; @@ -879,7 +872,7 @@ struct boot_params *efi_main(efi_handle_t handle, } raw_local_irq_disable(); - native_load_gdt(gdt); + native_load_gdt(&gdt); return boot_params; fail: -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/8] efi/x86: Factor GDT setup code into a function 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar 2020-01-30 20:04 ` [PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly Arvind Sankar 2020-01-30 20:04 ` [PATCH 2/8] efi/x86: Allocate the GDT pointer on the stack Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 4/8] efi/x86: Only setup the GDT for 32-bit kernel Arvind Sankar ` (6 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc Move GDT allocation and definition code into a function in preparation to cleaning it up a bit. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 191 ++++++++++++++++--------------- 1 file changed, 101 insertions(+), 90 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index a0a2fd0528af..066125a18391 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -344,6 +344,105 @@ void setup_graphics(struct boot_params *boot_params) } } +static efi_status_t setup_gdt(struct desc_ptr *gdt) +{ + efi_status_t status; + struct desc_struct *desc; + + gdt->size = 0x800; + status = efi_low_alloc(gdt->size, 8, (unsigned long *)&gdt->address); + if (status != EFI_SUCCESS) { + efi_printk("Failed to allocate memory for 'gdt'\n"); + return status; + } + + memset((char *)gdt->address, 0x0, gdt->size); + desc = (struct desc_struct *)gdt->address; + + /* The first GDT is a dummy. */ + desc++; + + if (IS_ENABLED(CONFIG_X86_64)) { + /* __KERNEL32_CS */ + desc->limit0 = 0xffff; + desc->base0 = 0x0000; + desc->base1 = 0x0000; + desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; + desc->s = DESC_TYPE_CODE_DATA; + desc->dpl = 0; + desc->p = 1; + desc->limit1 = 0xf; + desc->avl = 0; + desc->l = 0; + desc->d = SEG_OP_SIZE_32BIT; + desc->g = SEG_GRANULARITY_4KB; + desc->base2 = 0x00; + + desc++; + } else { + /* Second entry is unused on 32-bit */ + desc++; + } + + /* __KERNEL_CS */ + desc->limit0 = 0xffff; + desc->base0 = 0x0000; + desc->base1 = 0x0000; + desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; + desc->s = DESC_TYPE_CODE_DATA; + desc->dpl = 0; + desc->p = 1; + desc->limit1 = 0xf; + desc->avl = 0; + + if (IS_ENABLED(CONFIG_X86_64)) { + desc->l = 1; + desc->d = 0; + } else { + desc->l = 0; + desc->d = SEG_OP_SIZE_32BIT; + } + desc->g = SEG_GRANULARITY_4KB; + desc->base2 = 0x00; + desc++; + + /* __KERNEL_DS */ + desc->limit0 = 0xffff; + desc->base0 = 0x0000; + desc->base1 = 0x0000; + desc->type = SEG_TYPE_DATA | SEG_TYPE_READ_WRITE; + desc->s = DESC_TYPE_CODE_DATA; + desc->dpl = 0; + desc->p = 1; + desc->limit1 = 0xf; + desc->avl = 0; + desc->l = 0; + desc->d = SEG_OP_SIZE_32BIT; + desc->g = SEG_GRANULARITY_4KB; + desc->base2 = 0x00; + desc++; + + if (IS_ENABLED(CONFIG_X86_64)) { + /* Task segment value */ + desc->limit0 = 0x0000; + desc->base0 = 0x0000; + desc->base1 = 0x0000; + desc->type = SEG_TYPE_TSS; + desc->s = 0; + desc->dpl = 0; + desc->p = 1; + desc->limit1 = 0x0; + desc->avl = 0; + desc->l = 0; + desc->d = 0; + desc->g = SEG_GRANULARITY_4KB; + desc->base2 = 0x00; + desc++; + } + + return EFI_SUCCESS; +} + void startup_32(struct boot_params *boot_params); void __noreturn efi_stub_entry(efi_handle_t handle, @@ -716,7 +815,6 @@ struct boot_params *efi_main(efi_handle_t handle, struct desc_ptr gdt; struct setup_header *hdr = &boot_params->hdr; efi_status_t status; - struct desc_struct *desc; unsigned long cmdline_paddr; sys_table = sys_table_arg; @@ -754,12 +852,9 @@ struct boot_params *efi_main(efi_handle_t handle, setup_quirks(boot_params); - gdt.size = 0x800; - status = efi_low_alloc(gdt.size, 8, (unsigned long *)&gdt.address); - if (status != EFI_SUCCESS) { - efi_printk("Failed to allocate memory for 'gdt'\n"); + status = setup_gdt(&gdt); + if (status != EFI_SUCCESS) goto fail; - } /* * If the kernel isn't already loaded at the preferred load @@ -787,90 +882,6 @@ struct boot_params *efi_main(efi_handle_t handle, goto fail; } - memset((char *)gdt.address, 0x0, gdt.size); - desc = (struct desc_struct *)gdt.address; - - /* The first GDT is a dummy. */ - desc++; - - if (IS_ENABLED(CONFIG_X86_64)) { - /* __KERNEL32_CS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - - desc++; - } else { - /* Second entry is unused on 32-bit */ - desc++; - } - - /* __KERNEL_CS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - - if (IS_ENABLED(CONFIG_X86_64)) { - desc->l = 1; - desc->d = 0; - } else { - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - } - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - - /* __KERNEL_DS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_DATA | SEG_TYPE_READ_WRITE; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - - if (IS_ENABLED(CONFIG_X86_64)) { - /* Task segment value */ - desc->limit0 = 0x0000; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_TSS; - desc->s = 0; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0x0; - desc->avl = 0; - desc->l = 0; - desc->d = 0; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - } - raw_local_irq_disable(); native_load_gdt(&gdt); -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/8] efi/x86: Only setup the GDT for 32-bit kernel 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (2 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 3/8] efi/x86: Factor GDT setup code into a function Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 5/8] efi/x86: Allocate only the required 32 bytes for the GDT Arvind Sankar ` (5 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc On a 64-bit kernel, the next function to execute after efi_main returns is startup_64, which sets up its own GDT, so setting one up in efi_main is unnecessary. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 60 ++++++-------------------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 066125a18391..a72cfda91d7e 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -346,6 +346,7 @@ void setup_graphics(struct boot_params *boot_params) static efi_status_t setup_gdt(struct desc_ptr *gdt) { +#ifdef CONFIG_X86_32 efi_status_t status; struct desc_struct *desc; @@ -361,30 +362,10 @@ static efi_status_t setup_gdt(struct desc_ptr *gdt) /* The first GDT is a dummy. */ desc++; + /* Second entry is unused on 32-bit */ + desc++; - if (IS_ENABLED(CONFIG_X86_64)) { - /* __KERNEL32_CS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - - desc++; - } else { - /* Second entry is unused on 32-bit */ - desc++; - } - - /* __KERNEL_CS */ + /* __BOOT_CS */ desc->limit0 = 0xffff; desc->base0 = 0x0000; desc->base1 = 0x0000; @@ -394,19 +375,13 @@ static efi_status_t setup_gdt(struct desc_ptr *gdt) desc->p = 1; desc->limit1 = 0xf; desc->avl = 0; - - if (IS_ENABLED(CONFIG_X86_64)) { - desc->l = 1; - desc->d = 0; - } else { - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - } + desc->l = 0; + desc->d = SEG_OP_SIZE_32BIT; desc->g = SEG_GRANULARITY_4KB; desc->base2 = 0x00; desc++; - /* __KERNEL_DS */ + /* __BOOT_DS */ desc->limit0 = 0xffff; desc->base0 = 0x0000; desc->base1 = 0x0000; @@ -422,23 +397,7 @@ static efi_status_t setup_gdt(struct desc_ptr *gdt) desc->base2 = 0x00; desc++; - if (IS_ENABLED(CONFIG_X86_64)) { - /* Task segment value */ - desc->limit0 = 0x0000; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_TSS; - desc->s = 0; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0x0; - desc->avl = 0; - desc->l = 0; - desc->d = 0; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - } +#endif /* CONFIG_X86_32 */ return EFI_SUCCESS; } @@ -883,7 +842,8 @@ struct boot_params *efi_main(efi_handle_t handle, } raw_local_irq_disable(); - native_load_gdt(&gdt); + if (IS_ENABLED(CONFIG_X86_32)) + native_load_gdt(&gdt); return boot_params; fail: -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 5/8] efi/x86: Allocate only the required 32 bytes for the GDT 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (3 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 4/8] efi/x86: Only setup the GDT for 32-bit kernel Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 6/8] efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} Arvind Sankar ` (4 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc For the 32-bit boot GDT, we only need 4 descriptors. Allocate the required 32 bytes using allocate_pool, rather than always allocating a full page. The UEFI spec guarantees that allocate_pool returns an 8-byte aligned buffer, so we don't need to do additional alignment. The "size" stored in the GDT pointer should be one less than the true size of the GDT. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index a72cfda91d7e..2447e4508aa4 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -350,14 +350,23 @@ static efi_status_t setup_gdt(struct desc_ptr *gdt) efi_status_t status; struct desc_struct *desc; - gdt->size = 0x800; - status = efi_low_alloc(gdt->size, 8, (unsigned long *)&gdt->address); + gdt->size = 0x20; + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, gdt->size, + (void **)&gdt->address); if (status != EFI_SUCCESS) { efi_printk("Failed to allocate memory for 'gdt'\n"); return status; } memset((char *)gdt->address, 0x0, gdt->size); + + /* + * The "size" stored in the GDT pointer is actually a limit value, + * which when added to the base address gives the address of the last + * byte of the GDT. Hence it should be one less than the true size. + */ + gdt->size--; + desc = (struct desc_struct *)gdt->address; /* The first GDT is a dummy. */ -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 6/8] efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (4 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 5/8] efi/x86: Allocate only the required 32 bytes for the GDT Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover Arvind Sankar ` (3 subsequent siblings) 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc Although these are the same values, the 32-bit GDT was the one saved on entry from the bootloader, and it is more accurate to refer to __BOOT_{CS,DS} descriptors, rather than __KERNEL_{CS,DS} which are the descriptors in the 64-bit GDT. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/efi_thunk_64.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S index 8fb7f6799c52..4f32f05cbac6 100644 --- a/arch/x86/boot/compressed/efi_thunk_64.S +++ b/arch/x86/boot/compressed/efi_thunk_64.S @@ -58,7 +58,7 @@ SYM_FUNC_START(__efi64_thunk) leaq efi32_boot_gdt(%rip), %rax lgdt (%rax) - pushq $__KERNEL_CS + pushq $__BOOT_CS leaq efi_enter32(%rip), %rax pushq %rax lretq @@ -92,7 +92,7 @@ SYM_FUNC_END(__efi64_thunk) * The stack should represent the 32-bit calling convention. */ SYM_FUNC_START_LOCAL(efi_enter32) - movl $__KERNEL_DS, %eax + movl $__BOOT_DS, %eax movl %eax, %ds movl %eax, %es movl %eax, %ss -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (5 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 6/8] efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-31 19:24 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 8/8] Documentation/x86/boot: Correct segment requirements for 64-bit boot Arvind Sankar ` (2 subsequent siblings) 9 siblings, 1 reply; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc The 32-bit EFI handover entry point requires segments to be setup in the same way as for the regular 32-bit boot. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/x86/boot.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst index c9c201596c3e..3e13b7d57271 100644 --- a/Documentation/x86/boot.rst +++ b/Documentation/x86/boot.rst @@ -1412,6 +1412,12 @@ from the boot media and jump to the EFI handover protocol entry point which is hdr->handover_offset bytes from the beginning of startup_{32,64}. +For the 32-bit handover entry point, the GDT and segments must be setup as for +the 32-bit boot protocol, i.e. a GDT must be loaded with the descriptors for +selectors __BOOT_CS(0x10) and __BOOT_DS(0x18); both descriptors must be 4G flat +segment; __BOOT_CS must have execute/read permission, and __BOOT_DS must have +read/write permission; CS must be __BOOT_CS and DS, ES, SS must be __BOOT_DS. + The function prototype for the handover entry point looks like this:: efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp) -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover 2020-01-30 20:04 ` [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover Arvind Sankar @ 2020-01-31 19:24 ` Arvind Sankar 0 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-31 19:24 UTC (permalink / raw) To: Arvind Sankar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel, linux-efi, x86, linux-doc On Thu, Jan 30, 2020 at 03:04:39PM -0500, Arvind Sankar wrote: > The 32-bit EFI handover entry point requires segments to be setup in the > same way as for the regular 32-bit boot. > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> > --- > Documentation/x86/boot.rst | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst > index c9c201596c3e..3e13b7d57271 100644 > --- a/Documentation/x86/boot.rst > +++ b/Documentation/x86/boot.rst > @@ -1412,6 +1412,12 @@ from the boot media and jump to the EFI handover protocol entry point > which is hdr->handover_offset bytes from the beginning of > startup_{32,64}. > > +For the 32-bit handover entry point, the GDT and segments must be setup as for > +the 32-bit boot protocol, i.e. a GDT must be loaded with the descriptors for > +selectors __BOOT_CS(0x10) and __BOOT_DS(0x18); both descriptors must be 4G flat > +segment; __BOOT_CS must have execute/read permission, and __BOOT_DS must have > +read/write permission; CS must be __BOOT_CS and DS, ES, SS must be __BOOT_DS. > + > The function prototype for the handover entry point looks like this:: > > efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp) > -- > 2.24.1 > I realized this is actually wrong. At the handover entry, the firmware is still in control, so we can't require the bootloader to install a different GDT from what the firmware had installed, and in fact OVMF for one just passes the firmware GDT. This seems to be working today by accident. OVMF sets up a GDT which looks like this: NULL DATA CODE32 DATA CODE32 0 DATA CODE64 0 and this is what is installed when efi_stub_entry is called. Since selectors 2 and 3 are code and data this works for us in 32-bit and in mixed mode, and we don't care in 64-bit mode, but it seems like a bad thing to rely upon. I think we should save the segment descriptors in addition to the GDTR on stub entry and restore them in the mixed-mode thunk before calling the firmware? ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 8/8] Documentation/x86/boot: Correct segment requirements for 64-bit boot 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (6 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover Arvind Sankar @ 2020-01-30 20:04 ` Arvind Sankar 2020-01-31 8:42 ` [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Ard Biesheuvel 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar 9 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-30 20:04 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel Cc: linux-efi, x86, linux-doc 64-bit mode has no segment/GDT requirements as it does not really use segment registers. The entry code loads null descriptors into the data and stack segment registers. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- Documentation/x86/boot.rst | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst index 3e13b7d57271..df2bf8abbbc1 100644 --- a/Documentation/x86/boot.rst +++ b/Documentation/x86/boot.rst @@ -1396,12 +1396,9 @@ In 64-bit boot protocol, the kernel is started by jumping to the At entry, the CPU must be in 64-bit mode with paging enabled. The range with setup_header.init_size from start address of loaded kernel and zero page and command line buffer get ident mapping; -a GDT must be loaded with the descriptors for selectors -__BOOT_CS(0x10) and __BOOT_DS(0x18); both descriptors must be 4G flat -segment; __BOOT_CS must have execute/read permission, and __BOOT_DS -must have read/write permission; CS must be __BOOT_CS and DS, ES, SS -must be __BOOT_DS; interrupt must be disabled; %rsi must hold the base -address of the struct boot_params. +interrupt must be disabled; %rsi must hold the base address of the +struct boot_params. As 64-bit mode does not really use segments, there +are no special requirements on the segment registers or descriptors. EFI Handover Protocol ===================== -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (7 preceding siblings ...) 2020-01-30 20:04 ` [PATCH 8/8] Documentation/x86/boot: Correct segment requirements for 64-bit boot Arvind Sankar @ 2020-01-31 8:42 ` Ard Biesheuvel 2020-01-31 9:31 ` Ard Biesheuvel 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar 9 siblings, 1 reply; 24+ messages in thread From: Ard Biesheuvel @ 2020-01-31 8:42 UTC (permalink / raw) To: Arvind Sankar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Doc Mailing List Thanks Arvind, this is good stuff. On Thu, 30 Jan 2020 at 21:04, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > For the 64-bit kernel, we do not need to setup a GDT in efi_main, as the > next step in the boot, startup_64, will set one up. > This seems obvious, but I need a nod from Ingo or Boris before I can take this. > This series factors out the GDT setup into a separate function and > restricts it to the 32-bit kernel. The memory allocation for the GDT is > also changed from allocating a full page via efi_alloc_low to the > required space (32 bytes) from alloc_pool boot service. > Can we go all the way and simply make this if (IS_ENABLED(CONFIG_X64_32)) { static const struct desc_struct desc[] __aligned(8) = { {}, /* The first GDT is a dummy. */ {}, /* Second entry is unused on 32-bit */ GDT_ENTRY_INIT(0xa09b, 0, 0xfffff), /* __KERNEL_CS */ GDT_ENTRY_INIT(0xc093, 0, 0xfffff), /* __KERNEL_DS */ }; struct desc_ptr gdt = { sizeof(desc) - 1, (unsigned long)desc }; native_load_gdt(&gdt); } (and drop the #defines from eboot.h that we no longer use) > The final two patches are doc fixes to clarify that the 32-bit EFI > handover entry point also requires segments/GDT to be setup, and that > for 64-bit mode we don't have any special segment requirements (the > 64-bit doc currently is just a copy of the 32-bit doc which isn't > right). > > Arvind Sankar (8): > efi/x86: Use C wrapper instead of inline assembly > efi/x86: Allocate the GDT pointer on the stack > efi/x86: Factor GDT setup code into a function Given the above, I don't think we need the separate function, but if we do, please drop the 64-bit code first, otherwise there is much more churn than necessary. > efi/x86: Only setup the GDT for 32-bit kernel > efi/x86: Allocate only the required 32 bytes for the GDT > efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} > Documentation/x86/boot: Clarify segment requirements for EFI handover > Documentation/x86/boot: Correct segment requirements for 64-bit boot > > Documentation/x86/boot.rst | 15 +- > arch/x86/boot/compressed/eboot.c | 174 ++++++++++-------------- > arch/x86/boot/compressed/efi_thunk_64.S | 4 +- > 3 files changed, 85 insertions(+), 108 deletions(-) > > -- > 2.24.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes 2020-01-31 8:42 ` [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Ard Biesheuvel @ 2020-01-31 9:31 ` Ard Biesheuvel 2020-01-31 19:10 ` Arvind Sankar 0 siblings, 1 reply; 24+ messages in thread From: Ard Biesheuvel @ 2020-01-31 9:31 UTC (permalink / raw) To: Arvind Sankar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Doc Mailing List On Fri, 31 Jan 2020 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Thanks Arvind, this is good stuff. > > > On Thu, 30 Jan 2020 at 21:04, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > > > For the 64-bit kernel, we do not need to setup a GDT in efi_main, as the > > next step in the boot, startup_64, will set one up. > > > > This seems obvious, but I need a nod from Ingo or Boris before I can take this. > > > This series factors out the GDT setup into a separate function and > > restricts it to the 32-bit kernel. The memory allocation for the GDT is > > also changed from allocating a full page via efi_alloc_low to the > > required space (32 bytes) from alloc_pool boot service. > > > > Can we go all the way and simply make this > > if (IS_ENABLED(CONFIG_X64_32)) { > static const struct desc_struct desc[] __aligned(8) = { > {}, /* The first GDT is a dummy. */ > {}, /* Second entry is unused on 32-bit */ > GDT_ENTRY_INIT(0xa09b, 0, 0xfffff), /* __KERNEL_CS */ > GDT_ENTRY_INIT(0xc093, 0, 0xfffff), /* __KERNEL_DS */ > }; > struct desc_ptr gdt = { sizeof(desc) - 1, (unsigned long)desc }; > > native_load_gdt(&gdt); > } > > (and drop the #defines from eboot.h that we no longer use) > Playing around with this a bit more, I think we should go with if (IS_ENABLED(CONFIG_X86_32)) { static const struct desc_struct desc[] __aligned(8) = { [GDT_ENTRY_BOOT_CS] = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff), [GDT_ENTRY_BOOT_DS] = GDT_ENTRY_INIT(0xc093, 0, 0xfffff), }; native_load_gdt(&(struct desc_ptr){ sizeof(desc) - 1, (unsigned long)desc }); } > > The final two patches are doc fixes to clarify that the 32-bit EFI > > handover entry point also requires segments/GDT to be setup, and that > > for 64-bit mode we don't have any special segment requirements (the > > 64-bit doc currently is just a copy of the 32-bit doc which isn't > > right). > > > > Arvind Sankar (8): > > efi/x86: Use C wrapper instead of inline assembly > > efi/x86: Allocate the GDT pointer on the stack > > efi/x86: Factor GDT setup code into a function > > Given the above, I don't think we need the separate function, but if > we do, please drop the 64-bit code first, otherwise there is much more > churn than necessary. > > > efi/x86: Only setup the GDT for 32-bit kernel > > efi/x86: Allocate only the required 32 bytes for the GDT > > efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} > > Documentation/x86/boot: Clarify segment requirements for EFI handover > > Documentation/x86/boot: Correct segment requirements for 64-bit boot > > > > Documentation/x86/boot.rst | 15 +- > > arch/x86/boot/compressed/eboot.c | 174 ++++++++++-------------- > > arch/x86/boot/compressed/efi_thunk_64.S | 4 +- > > 3 files changed, 85 insertions(+), 108 deletions(-) > > > > -- > > 2.24.1 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes 2020-01-31 9:31 ` Ard Biesheuvel @ 2020-01-31 19:10 ` Arvind Sankar 0 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-01-31 19:10 UTC (permalink / raw) To: Ard Biesheuvel Cc: Arvind Sankar, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jonathan Corbet, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Doc Mailing List On Fri, Jan 31, 2020 at 10:31:44AM +0100, Ard Biesheuvel wrote: > Playing around with this a bit more, I think we should go with > > if (IS_ENABLED(CONFIG_X86_32)) { > static const struct desc_struct desc[] __aligned(8) = { > [GDT_ENTRY_BOOT_CS] = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff), > [GDT_ENTRY_BOOT_DS] = GDT_ENTRY_INIT(0xc093, 0, 0xfffff), > }; > > native_load_gdt(&(struct desc_ptr){ sizeof(desc) - 1, > (unsigned long)desc }); > } > This is the way I originally did it (except I loaded the GDT with the relocated address of desc in case efi_relocate_kernel got called). It booted on a test, but I grew concerned because this GDT will be somewhere in the middle of the decompression buffer and get overwritten either during the copy to the end of the buffer or during decompression. If anything tries to reload segment registers it will blow up. Looking at the code though it doesn't look like anything will -- the segments are loaded at the beginning of startup_32, and then it looks like nothing should touch them until we get to the startup code of the decompressed kernel, which sets up its own boot GDT first. So this might be ok if the x86 maintainers agree. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar ` (8 preceding siblings ...) 2020-01-31 8:42 ` [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Ard Biesheuvel @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support Arvind Sankar ` (7 more replies) 9 siblings, 8 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout Arvind Sankar ` (6 subsequent siblings) 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel Commit a24e785111a3 ("i386: paravirt boot sequence") added this flag for use by paravirtualized environments such as Xen. However, Xen never made use of this flag [1], and it was only ever used by lguest [2]. Commit ecda85e70277 ("x86/lguest: Remove lguest support") removed lguest, so KEEP_SEGMENTS has lost its last user. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> [1] https://lore.kernel.org/lkml/4D4B097C.5050405@goop.org [2] https://www.mail-archive.com/lguest@lists.ozlabs.org/msg00469.html --- Documentation/x86/boot.rst | 8 ++------ arch/x86/boot/compressed/head_32.S | 8 -------- arch/x86/boot/compressed/head_64.S | 8 -------- arch/x86/kernel/head_32.S | 6 ------ 4 files changed, 2 insertions(+), 28 deletions(-) diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst index c9c201596c3e..fa7ddc0428c8 100644 --- a/Documentation/x86/boot.rst +++ b/Documentation/x86/boot.rst @@ -490,15 +490,11 @@ Protocol: 2.00+ kernel) to not write early messages that require accessing the display hardware directly. - Bit 6 (write): KEEP_SEGMENTS + Bit 6 (obsolete): KEEP_SEGMENTS Protocol: 2.07+ - - If 0, reload the segment registers in the 32bit entry point. - - If 1, do not reload the segment registers in the 32bit entry point. - - Assume that %cs %ds %ss %es are all set to flat segments with - a base of 0 (or the equivalent for their environment). + - This flag is obsolete. Bit 7 (write): CAN_USE_HEAP diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S index 73f17d0544dd..cb2cb91fce45 100644 --- a/arch/x86/boot/compressed/head_32.S +++ b/arch/x86/boot/compressed/head_32.S @@ -63,13 +63,6 @@ __HEAD SYM_FUNC_START(startup_32) cld - /* - * Test KEEP_SEGMENTS flag to see if the bootloader is asking - * us to not reload segments - */ - testb $KEEP_SEGMENTS, BP_loadflags(%esi) - jnz 1f - cli movl $__BOOT_DS, %eax movl %eax, %ds @@ -77,7 +70,6 @@ SYM_FUNC_START(startup_32) movl %eax, %fs movl %eax, %gs movl %eax, %ss -1: /* * Calculate the delta between where we were compiled to run diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 1f1f6c8139b3..bd44d89540d3 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -53,19 +53,11 @@ SYM_FUNC_START(startup_32) * all need to be under the 4G limit. */ cld - /* - * Test KEEP_SEGMENTS flag to see if the bootloader is asking - * us to not reload segments - */ - testb $KEEP_SEGMENTS, BP_loadflags(%esi) - jnz 1f - cli movl $(__BOOT_DS), %eax movl %eax, %ds movl %eax, %es movl %eax, %ss -1: /* * Calculate the delta between where we were compiled to run diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S index 3923ab4630d7..f66a6b90f954 100644 --- a/arch/x86/kernel/head_32.S +++ b/arch/x86/kernel/head_32.S @@ -67,11 +67,6 @@ __HEAD SYM_CODE_START(startup_32) movl pa(initial_stack),%ecx - /* test KEEP_SEGMENTS flag to see if the bootloader is asking - us to not reload segments */ - testb $KEEP_SEGMENTS, BP_loadflags(%esi) - jnz 2f - /* * Set segments to known values. */ @@ -82,7 +77,6 @@ SYM_CODE_START(startup_32) movl %eax,%fs movl %eax,%gs movl %eax,%ss -2: leal -__PAGE_OFFSET(%ecx),%esp /* -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:54 ` Ard Biesheuvel 2020-02-02 17:13 ` [PATCH v2 3/7] x86/boot: Reload GDTR after copying to the end of the buffer Arvind Sankar ` (5 subsequent siblings) 7 siblings, 1 reply; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel At handover entry in efi32_stub_entry, the firmware's GDT is still installed. We save the GDTR for later use in __efi64_thunk but we are assuming that descriptor 2 (__KERNEL_CS) is a valid 32-bit code segment descriptor and that descriptor 3 (__KERNEL_DS/__BOOT_DS) is a valid data segment descriptor. This happens to be true for OVMF (it actually uses descriptor 1 for data segments, but descriptor 3 is also setup as data), but we shouldn't depend on this being the case. Fix this by saving the code and data selectors in addition to the GDTR in efi32_stub_entry, and restoring them in __efi64_thunk before calling the firmware. The UEFI specification guarantees that selectors will be flat, so using the DS selector for all the segment registers should be enough. We also need to install our own GDT before initializing segment registers in startup_32, so move the GDT load up to the beginning of the function. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/efi_thunk_64.S | 29 +++++++++++++++++++----- arch/x86/boot/compressed/head_64.S | 30 +++++++++++++++---------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S index 8fb7f6799c52..2b2049259619 100644 --- a/arch/x86/boot/compressed/efi_thunk_64.S +++ b/arch/x86/boot/compressed/efi_thunk_64.S @@ -54,11 +54,16 @@ SYM_FUNC_START(__efi64_thunk) * Switch to gdt with 32-bit segments. This is the firmware GDT * that was installed when the kernel started executing. This * pointer was saved at the EFI stub entry point in head_64.S. + * + * Pass the saved DS selector to the 32-bit code, and use far return to + * restore the saved CS selector. */ leaq efi32_boot_gdt(%rip), %rax lgdt (%rax) - pushq $__KERNEL_CS + movzwl efi32_boot_ds(%rip), %edx + movzwq efi32_boot_cs(%rip), %rax + pushq %rax leaq efi_enter32(%rip), %rax pushq %rax lretq @@ -73,6 +78,10 @@ SYM_FUNC_START(__efi64_thunk) movl %ebx, %es pop %rbx movl %ebx, %ds + /* Clear out 32-bit selector from FS and GS */ + xorl %ebx, %ebx + movl %ebx, %fs + movl %ebx, %gs /* * Convert 32-bit status code into 64-bit. @@ -92,10 +101,12 @@ SYM_FUNC_END(__efi64_thunk) * The stack should represent the 32-bit calling convention. */ SYM_FUNC_START_LOCAL(efi_enter32) - movl $__KERNEL_DS, %eax - movl %eax, %ds - movl %eax, %es - movl %eax, %ss + /* Load firmware selector into data and stack segment registers */ + movl %edx, %ds + movl %edx, %es + movl %edx, %fs + movl %edx, %gs + movl %edx, %ss /* Reload pgtables */ movl %cr3, %eax @@ -157,6 +168,14 @@ SYM_DATA_START(efi32_boot_gdt) .quad 0 SYM_DATA_END(efi32_boot_gdt) +SYM_DATA_START(efi32_boot_cs) + .word 0 +SYM_DATA_END(efi32_boot_cs) + +SYM_DATA_START(efi32_boot_ds) + .word 0 +SYM_DATA_END(efi32_boot_ds) + SYM_DATA_START(efi_gdt64) .word efi_gdt64_end - efi_gdt64 .long 0 /* Filled out by user */ diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index bd44d89540d3..c56b30bd9c7b 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -54,10 +54,6 @@ SYM_FUNC_START(startup_32) */ cld cli - movl $(__BOOT_DS), %eax - movl %eax, %ds - movl %eax, %es - movl %eax, %ss /* * Calculate the delta between where we were compiled to run @@ -72,10 +68,20 @@ SYM_FUNC_START(startup_32) 1: popl %ebp subl $1b, %ebp + /* Load new GDT with the 64bit segments using 32bit descriptor */ + addl %ebp, gdt+2(%ebp) + lgdt gdt(%ebp) + + /* Load segment registers with our descriptors */ + movl $__BOOT_DS, %eax + movl %eax, %ds + movl %eax, %es + movl %eax, %fs + movl %eax, %gs + movl %eax, %ss + /* setup a stack and make sure cpu supports long mode. */ - movl $boot_stack_end, %eax - addl %ebp, %eax - movl %eax, %esp + leal boot_stack_end(%ebp), %esp call verify_cpu testl %eax, %eax @@ -112,10 +118,6 @@ SYM_FUNC_START(startup_32) * Prepare for entering 64 bit mode */ - /* Load new GDT with the 64bit segments using 32bit descriptor */ - addl %ebp, gdt+2(%ebp) - lgdt gdt(%ebp) - /* Enable PAE mode */ movl %cr4, %eax orl $X86_CR4_PAE, %eax @@ -232,9 +234,13 @@ SYM_FUNC_START(efi32_stub_entry) movl %ecx, efi32_boot_args(%ebp) movl %edx, efi32_boot_args+4(%ebp) - sgdtl efi32_boot_gdt(%ebp) movb $0, efi_is64(%ebp) + /* Save firmware GDTR and code/data selectors */ + sgdtl efi32_boot_gdt(%ebp) + movw %cs, efi32_boot_cs(%ebp) + movw %ds, efi32_boot_ds(%ebp) + /* Disable paging */ movl %cr0, %eax btrl $X86_CR0_PG_BIT, %eax -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout 2020-02-02 17:13 ` [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout Arvind Sankar @ 2020-02-02 17:54 ` Ard Biesheuvel 2020-02-02 18:18 ` Arvind Sankar 0 siblings, 1 reply; 24+ messages in thread From: Ard Biesheuvel @ 2020-02-02 17:54 UTC (permalink / raw) To: Arvind Sankar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Kernel Mailing List On Sun, 2 Feb 2020 at 18:13, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > At handover entry in efi32_stub_entry, the firmware's GDT is still > installed. We save the GDTR for later use in __efi64_thunk but we are > assuming that descriptor 2 (__KERNEL_CS) is a valid 32-bit code segment > descriptor and that descriptor 3 (__KERNEL_DS/__BOOT_DS) is a valid data > segment descriptor. > > This happens to be true for OVMF (it actually uses descriptor 1 for data > segments, but descriptor 3 is also setup as data), but we shouldn't > depend on this being the case. > > Fix this by saving the code and data selectors in addition to the GDTR > in efi32_stub_entry, and restoring them in __efi64_thunk before calling > the firmware. The UEFI specification guarantees that selectors will be > flat, so using the DS selector for all the segment registers should be > enough. > > We also need to install our own GDT before initializing segment > registers in startup_32, so move the GDT load up to the beginning of the > function. > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> It might be useful to mention /somewhere/ in the commit log that this applies to mixed mode > --- > arch/x86/boot/compressed/efi_thunk_64.S | 29 +++++++++++++++++++----- > arch/x86/boot/compressed/head_64.S | 30 +++++++++++++++---------- > 2 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S > index 8fb7f6799c52..2b2049259619 100644 > --- a/arch/x86/boot/compressed/efi_thunk_64.S > +++ b/arch/x86/boot/compressed/efi_thunk_64.S > @@ -54,11 +54,16 @@ SYM_FUNC_START(__efi64_thunk) > * Switch to gdt with 32-bit segments. This is the firmware GDT > * that was installed when the kernel started executing. This > * pointer was saved at the EFI stub entry point in head_64.S. > + * > + * Pass the saved DS selector to the 32-bit code, and use far return to > + * restore the saved CS selector. > */ > leaq efi32_boot_gdt(%rip), %rax > lgdt (%rax) > > - pushq $__KERNEL_CS > + movzwl efi32_boot_ds(%rip), %edx > + movzwq efi32_boot_cs(%rip), %rax > + pushq %rax > leaq efi_enter32(%rip), %rax > pushq %rax > lretq > @@ -73,6 +78,10 @@ SYM_FUNC_START(__efi64_thunk) > movl %ebx, %es > pop %rbx > movl %ebx, %ds > + /* Clear out 32-bit selector from FS and GS */ > + xorl %ebx, %ebx > + movl %ebx, %fs > + movl %ebx, %gs > > /* > * Convert 32-bit status code into 64-bit. > @@ -92,10 +101,12 @@ SYM_FUNC_END(__efi64_thunk) > * The stack should represent the 32-bit calling convention. > */ > SYM_FUNC_START_LOCAL(efi_enter32) > - movl $__KERNEL_DS, %eax > - movl %eax, %ds > - movl %eax, %es > - movl %eax, %ss > + /* Load firmware selector into data and stack segment registers */ > + movl %edx, %ds > + movl %edx, %es > + movl %edx, %fs > + movl %edx, %gs > + movl %edx, %ss > > /* Reload pgtables */ > movl %cr3, %eax > @@ -157,6 +168,14 @@ SYM_DATA_START(efi32_boot_gdt) > .quad 0 > SYM_DATA_END(efi32_boot_gdt) > > +SYM_DATA_START(efi32_boot_cs) > + .word 0 > +SYM_DATA_END(efi32_boot_cs) > + > +SYM_DATA_START(efi32_boot_ds) > + .word 0 > +SYM_DATA_END(efi32_boot_ds) > + > SYM_DATA_START(efi_gdt64) > .word efi_gdt64_end - efi_gdt64 > .long 0 /* Filled out by user */ > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > index bd44d89540d3..c56b30bd9c7b 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -54,10 +54,6 @@ SYM_FUNC_START(startup_32) > */ > cld > cli > - movl $(__BOOT_DS), %eax > - movl %eax, %ds > - movl %eax, %es > - movl %eax, %ss > > /* > * Calculate the delta between where we were compiled to run > @@ -72,10 +68,20 @@ SYM_FUNC_START(startup_32) > 1: popl %ebp > subl $1b, %ebp > > + /* Load new GDT with the 64bit segments using 32bit descriptor */ > + addl %ebp, gdt+2(%ebp) > + lgdt gdt(%ebp) > + > + /* Load segment registers with our descriptors */ > + movl $__BOOT_DS, %eax > + movl %eax, %ds > + movl %eax, %es > + movl %eax, %fs > + movl %eax, %gs > + movl %eax, %ss > + > /* setup a stack and make sure cpu supports long mode. */ > - movl $boot_stack_end, %eax > - addl %ebp, %eax > - movl %eax, %esp > + leal boot_stack_end(%ebp), %esp > > call verify_cpu > testl %eax, %eax > @@ -112,10 +118,6 @@ SYM_FUNC_START(startup_32) > * Prepare for entering 64 bit mode > */ > > - /* Load new GDT with the 64bit segments using 32bit descriptor */ > - addl %ebp, gdt+2(%ebp) > - lgdt gdt(%ebp) > - > /* Enable PAE mode */ > movl %cr4, %eax > orl $X86_CR4_PAE, %eax > @@ -232,9 +234,13 @@ SYM_FUNC_START(efi32_stub_entry) > > movl %ecx, efi32_boot_args(%ebp) > movl %edx, efi32_boot_args+4(%ebp) > - sgdtl efi32_boot_gdt(%ebp) > movb $0, efi_is64(%ebp) > > + /* Save firmware GDTR and code/data selectors */ > + sgdtl efi32_boot_gdt(%ebp) > + movw %cs, efi32_boot_cs(%ebp) > + movw %ds, efi32_boot_ds(%ebp) > + > /* Disable paging */ > movl %cr0, %eax > btrl $X86_CR0_PG_BIT, %eax > -- > 2.24.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout 2020-02-02 17:54 ` Ard Biesheuvel @ 2020-02-02 18:18 ` Arvind Sankar 0 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 18:18 UTC (permalink / raw) To: Ard Biesheuvel Cc: Arvind Sankar, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Kernel Mailing List On Sun, Feb 02, 2020 at 06:54:48PM +0100, Ard Biesheuvel wrote: > On Sun, 2 Feb 2020 at 18:13, Arvind Sankar <nivedita@alum.mit.edu> wrote: > > > > At handover entry in efi32_stub_entry, the firmware's GDT is still > > installed. We save the GDTR for later use in __efi64_thunk but we are > > assuming that descriptor 2 (__KERNEL_CS) is a valid 32-bit code segment > > descriptor and that descriptor 3 (__KERNEL_DS/__BOOT_DS) is a valid data > > segment descriptor. > > > > This happens to be true for OVMF (it actually uses descriptor 1 for data > > segments, but descriptor 3 is also setup as data), but we shouldn't > > depend on this being the case. > > > > Fix this by saving the code and data selectors in addition to the GDTR > > in efi32_stub_entry, and restoring them in __efi64_thunk before calling > > the firmware. The UEFI specification guarantees that selectors will be > > flat, so using the DS selector for all the segment registers should be > > enough. > > > > We also need to install our own GDT before initializing segment > > registers in startup_32, so move the GDT load up to the beginning of the > > function. > > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> > > It might be useful to mention /somewhere/ in the commit log that this > applies to mixed mode > Good point. I'll wait for comments from the x86 guys and include that in the next re-spin. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 3/7] x86/boot: Reload GDTR after copying to the end of the buffer 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 4/7] x86/boot: Clear direction and interrupt flags in startup_64 Arvind Sankar ` (4 subsequent siblings) 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel The GDT may get overwritten during the copy or during extract_kernel, which will cause problems if any segment register is touched before the GDTR is reloaded by the decompressed kernel. For safety update the GDTR to point to the GDT within the copied kernel. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/head_64.S | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index c56b30bd9c7b..27eb2a6786db 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -439,6 +439,16 @@ trampoline_return: cld popq %rsi + /* + * The GDT may get overwritten either during the copy we just did or + * during extract_kernel below. To avoid any issues, repoint the GDTR + * to the new copy of the GDT. + */ + leaq gdt64(%rbx), %rax + subq %rbp, 2(%rax) + addq %rbx, 2(%rax) + lgdt (%rax) + /* * Jump to the relocated address. */ -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 4/7] x86/boot: Clear direction and interrupt flags in startup_64 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar ` (2 preceding siblings ...) 2020-02-02 17:13 ` [PATCH v2 3/7] x86/boot: Reload GDTR after copying to the end of the buffer Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 5/7] efi/x86: Remove GDT setup from efi_main Arvind Sankar ` (3 subsequent siblings) 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel startup_32 already clears these flags on entry, do it in startup_64 as well for consistency. The direction flag in particular is not specified to be cleared in the boot protocol documentation, and we currently call into C code (paging_prepare) without explicitly clearing it. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/head_64.S | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 27eb2a6786db..69cc6c68741e 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -264,6 +264,9 @@ SYM_CODE_START(startup_64) * and command line. */ + cld + cli + /* Setup data segments. */ xorl %eax, %eax movl %eax, %ds -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 5/7] efi/x86: Remove GDT setup from efi_main 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar ` (3 preceding siblings ...) 2020-02-02 17:13 ` [PATCH v2 4/7] x86/boot: Clear direction and interrupt flags in startup_64 Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 6/7] x86/boot: GDT limit value should be size - 1 Arvind Sankar ` (2 subsequent siblings) 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel The 64-bit kernel will already load a GDT in startup_64, which is the next function to execute after return from efi_main. Add GDT setup code to the 32-bit kernel's startup_32 as well. Doing it in the head code has the advantage that we can avoid potentially corrupting the GDT during copy/decompression. This also removes dependence on having a specific GDT layout setup by the bootloader. Both startup_32 and startup_64 now clear interrupts on entry, so we can remove that from efi_main as well. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/eboot.c | 103 ----------------------------- arch/x86/boot/compressed/head_32.S | 40 +++++++++-- 2 files changed, 34 insertions(+), 109 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 287393d725f0..c92fe0b75cec 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -712,10 +712,8 @@ struct boot_params *efi_main(efi_handle_t handle, efi_system_table_t *sys_table_arg, struct boot_params *boot_params) { - struct desc_ptr *gdt = NULL; struct setup_header *hdr = &boot_params->hdr; efi_status_t status; - struct desc_struct *desc; unsigned long cmdline_paddr; sys_table = sys_table_arg; @@ -753,20 +751,6 @@ struct boot_params *efi_main(efi_handle_t handle, setup_quirks(boot_params); - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*gdt), - (void **)&gdt); - if (status != EFI_SUCCESS) { - efi_printk("Failed to allocate memory for 'gdt' structure\n"); - goto fail; - } - - gdt->size = 0x800; - status = efi_low_alloc(gdt->size, 8, (unsigned long *)&gdt->address); - if (status != EFI_SUCCESS) { - efi_printk("Failed to allocate memory for 'gdt'\n"); - goto fail; - } - /* * If the kernel isn't already loaded at the preferred load * address, relocate it. @@ -793,93 +777,6 @@ struct boot_params *efi_main(efi_handle_t handle, goto fail; } - memset((char *)gdt->address, 0x0, gdt->size); - desc = (struct desc_struct *)gdt->address; - - /* The first GDT is a dummy. */ - desc++; - - if (IS_ENABLED(CONFIG_X86_64)) { - /* __KERNEL32_CS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - - desc++; - } else { - /* Second entry is unused on 32-bit */ - desc++; - } - - /* __KERNEL_CS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - - if (IS_ENABLED(CONFIG_X86_64)) { - desc->l = 1; - desc->d = 0; - } else { - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - } - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - - /* __KERNEL_DS */ - desc->limit0 = 0xffff; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_DATA | SEG_TYPE_READ_WRITE; - desc->s = DESC_TYPE_CODE_DATA; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0xf; - desc->avl = 0; - desc->l = 0; - desc->d = SEG_OP_SIZE_32BIT; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - - if (IS_ENABLED(CONFIG_X86_64)) { - /* Task segment value */ - desc->limit0 = 0x0000; - desc->base0 = 0x0000; - desc->base1 = 0x0000; - desc->type = SEG_TYPE_TSS; - desc->s = 0; - desc->dpl = 0; - desc->p = 1; - desc->limit1 = 0x0; - desc->avl = 0; - desc->l = 0; - desc->d = 0; - desc->g = SEG_GRANULARITY_4KB; - desc->base2 = 0x00; - desc++; - } - - asm volatile("cli"); - asm volatile ("lgdt %0" : : "m" (*gdt)); - return boot_params; fail: efi_printk("efi_main() failed!\n"); diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S index cb2cb91fce45..356060c5332c 100644 --- a/arch/x86/boot/compressed/head_32.S +++ b/arch/x86/boot/compressed/head_32.S @@ -64,12 +64,6 @@ SYM_FUNC_START(startup_32) cld cli - movl $__BOOT_DS, %eax - movl %eax, %ds - movl %eax, %es - movl %eax, %fs - movl %eax, %gs - movl %eax, %ss /* * Calculate the delta between where we were compiled to run @@ -84,6 +78,19 @@ SYM_FUNC_START(startup_32) 1: popl %ebp subl $1b, %ebp + /* Load new GDT */ + leal gdt(%ebp), %eax + movl %eax, 2(%eax) + lgdt (%eax) + + /* Load segment registers with our descriptors */ + movl $__BOOT_DS, %eax + movl %eax, %ds + movl %eax, %es + movl %eax, %fs + movl %eax, %gs + movl %eax, %ss + /* * %ebp contains the address we are loaded at by the boot loader and %ebx * contains the address where we should move the kernel image temporarily @@ -129,6 +136,16 @@ SYM_FUNC_START(startup_32) cld popl %esi + /* + * The GDT may get overwritten either during the copy we just did or + * during extract_kernel below. To avoid any issues, repoint the GDTR + * to the new copy of the GDT. EAX still contains the previously + * calculated relocation offset of init_size - _end. + */ + leal gdt(%ebx), %edx + addl %eax, 2(%edx) + lgdt (%edx) + /* * Jump to the relocated address. */ @@ -201,6 +218,17 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated) jmp *%eax SYM_FUNC_END(.Lrelocated) + .data + .balign 8 +SYM_DATA_START_LOCAL(gdt) + .word gdt_end - gdt - 1 + .long 0 + .word 0 + .quad 0x0000000000000000 /* Reserved */ + .quad 0x00cf9a000000ffff /* __KERNEL_CS */ + .quad 0x00cf92000000ffff /* __KERNEL_DS */ +SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end) + /* * Stack and heap for uncompression */ -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 6/7] x86/boot: GDT limit value should be size - 1 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar ` (4 preceding siblings ...) 2020-02-02 17:13 ` [PATCH v2 5/7] efi/x86: Remove GDT setup from efi_main Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 7/7] x86/boot: Micro-optimize GDT loading instructions Arvind Sankar 2020-02-02 18:01 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Ard Biesheuvel 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel The limit value for the GDTR should be such that adding it to the base address gives the address of the last byte of the GDT, i.e. it should be one less than the size, not the size. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> --- arch/x86/boot/compressed/head_64.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 69cc6c68741e..c36e6156b6a3 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -624,12 +624,12 @@ SYM_FUNC_END(.Lno_longmode) .data SYM_DATA_START_LOCAL(gdt64) - .word gdt_end - gdt + .word gdt_end - gdt - 1 .quad 0 SYM_DATA_END(gdt64) .balign 8 SYM_DATA_START_LOCAL(gdt) - .word gdt_end - gdt + .word gdt_end - gdt - 1 .long gdt .word 0 .quad 0x00cf9a000000ffff /* __KERNEL32_CS */ -- 2.24.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 7/7] x86/boot: Micro-optimize GDT loading instructions 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar ` (5 preceding siblings ...) 2020-02-02 17:13 ` [PATCH v2 6/7] x86/boot: GDT limit value should be size - 1 Arvind Sankar @ 2020-02-02 17:13 ` Arvind Sankar 2020-02-02 18:01 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Ard Biesheuvel 7 siblings, 0 replies; 24+ messages in thread From: Arvind Sankar @ 2020-02-02 17:13 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel Cc: linux-efi, x86, linux-kernel 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 <nivedita@alum.mit.edu> --- 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar ` (6 preceding siblings ...) 2020-02-02 17:13 ` [PATCH v2 7/7] x86/boot: Micro-optimize GDT loading instructions Arvind Sankar @ 2020-02-02 18:01 ` Ard Biesheuvel 7 siblings, 0 replies; 24+ messages in thread From: Ard Biesheuvel @ 2020-02-02 18:01 UTC (permalink / raw) To: Arvind Sankar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Ard Biesheuvel, linux-efi, the arch/x86 maintainers, Linux Kernel Mailing List On Sun, 2 Feb 2020 at 18:13, Arvind Sankar <nivedita@alum.mit.edu> 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 <ardb@kernel.org> 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-02-02 18:18 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-30 20:04 [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Arvind Sankar 2020-01-30 20:04 ` [PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly Arvind Sankar 2020-01-30 20:04 ` [PATCH 2/8] efi/x86: Allocate the GDT pointer on the stack Arvind Sankar 2020-01-30 20:04 ` [PATCH 3/8] efi/x86: Factor GDT setup code into a function Arvind Sankar 2020-01-30 20:04 ` [PATCH 4/8] efi/x86: Only setup the GDT for 32-bit kernel Arvind Sankar 2020-01-30 20:04 ` [PATCH 5/8] efi/x86: Allocate only the required 32 bytes for the GDT Arvind Sankar 2020-01-30 20:04 ` [PATCH 6/8] efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS} Arvind Sankar 2020-01-30 20:04 ` [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover Arvind Sankar 2020-01-31 19:24 ` Arvind Sankar 2020-01-30 20:04 ` [PATCH 8/8] Documentation/x86/boot: Correct segment requirements for 64-bit boot Arvind Sankar 2020-01-31 8:42 ` [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes Ard Biesheuvel 2020-01-31 9:31 ` Ard Biesheuvel 2020-01-31 19:10 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout Arvind Sankar 2020-02-02 17:54 ` Ard Biesheuvel 2020-02-02 18:18 ` Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 3/7] x86/boot: Reload GDTR after copying to the end of the buffer Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 4/7] x86/boot: Clear direction and interrupt flags in startup_64 Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 5/7] efi/x86: Remove GDT setup from efi_main Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 6/7] x86/boot: GDT limit value should be size - 1 Arvind Sankar 2020-02-02 17:13 ` [PATCH v2 7/7] x86/boot: Micro-optimize GDT loading instructions Arvind Sankar 2020-02-02 18:01 ` [PATCH v2 0/7] x86/efi,boot: GDT handling cleanup/fixes Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).