[PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes

[PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes

[Arvind Sankar]

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

[PATCH 1/8] efi/x86: Use C wrapper instead of inline assembly

[Arvind Sankar]

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

[PATCH 2/8] efi/x86: Allocate the GDT pointer on the stack

[Arvind Sankar]

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

[PATCH 3/8] efi/x86: Factor GDT setup code into a function

[Arvind Sankar]

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

[PATCH 4/8] efi/x86: Only setup the GDT for 32-bit kernel

[Arvind Sankar]

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

[PATCH 5/8] efi/x86: Allocate only the required 32 bytes for the GDT

[Arvind Sankar]

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

[PATCH 6/8] efi/x86: Change __KERNEL_{CS,DS} to __BOOT_{CS,DS}

[Arvind Sankar]

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

[PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover

[Arvind Sankar]

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

Re: [PATCH 7/8] Documentation/x86/boot: Clarify segment requirements for EFI handover

[Arvind Sankar]

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?

[PATCH 8/8] Documentation/x86/boot: Correct segment requirements for 64-bit boot

[Arvind Sankar]

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

Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes

[Ard Biesheuvel]

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
>

Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes

[Ard Biesheuvel]

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
> >

Re: [PATCH 0/8] Remove 64-bit GDT setup in efi_main + doc fixes

[Arvind Sankar]

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.

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

[Arvind Sankar]

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

[PATCH v2 1/7] x86/boot: Remove KEEP_SEGMENTS support

[Arvind Sankar]

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

[PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout

[Arvind Sankar]

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

Re: [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout

[Ard Biesheuvel]

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
>

Re: [PATCH v2 2/7] efi/x86: Don't depend on firmware GDT layout

[Arvind Sankar]

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.

[PATCH v2 3/7] x86/boot: Reload GDTR after copying to the end of the buffer

[Arvind Sankar]

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

[PATCH v2 4/7] x86/boot: Clear direction and interrupt flags in startup_64

[Arvind Sankar]

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

[PATCH v2 5/7] efi/x86: Remove GDT setup from efi_main

[Arvind Sankar]

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

[PATCH v2 6/7] x86/boot: GDT limit value should be size - 1

[Arvind Sankar]

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

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

[Arvind Sankar]

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

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

[Ard Biesheuvel]

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
>