Attachment 225480 Details for Bug 209821 – [PATCH] efi: loader: Fix a boot freeze on some amd64 systems

[patch] [PATCH] efi: loader: Fix a boot freeze on some amd64 systems

0001-efi-loader-Fix-a-boot-freeze-on-some-amd64-systems.patch (text/plain), 11.36 KB, created by David Sebek on 2021-06-01 20:56:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Sebek

Created: 2021-06-01 20:56:43 UTC

Size: 11.36 KB

patch

obsolete

>From ec1d8aff437117ca6c4b0d93107499ae038f94e5 Mon Sep 17 00:00:00 2001
>From: David Sebek <dasebek@gmail.com>
>Date: Tue, 1 Jun 2021 16:32:26 -0400
>Subject: [PATCH] efi: loader: Fix a boot freeze on some amd64 systems
>
>Fix an issue when the boot process could freeze shortly after
>displaying EFI framebuffer information on some amd64 systems.
>
>Instead of calling the efi_copy_finish function from amd64_tramp,
>include the copy instructions in the trampoline code itself.
>This avoids boot hangs and restarts in the cases when
>the EFI loader code and/or data segments happen to be located
>at a memory location that is overwritten during the copy process.
>
>Also, add missing return value checks. Add a new function that
>prints a warning message if there is a danger that the bootloader
>may attempt to overwrite memory pages of a type that should not
>be overwritten.
>
>Signed-off-by: David Sebek <dasebek@gmail.com>
>---
> stand/efi/loader/arch/amd64/amd64_tramp.S   | 52 ++++++++++++
> stand/efi/loader/arch/amd64/elf64_freebsd.c | 42 +++++++---
> stand/efi/loader/bootinfo.c                 |  6 +-
> stand/efi/loader/copy.c                     | 89 ++++++++++++++++++++-
> stand/efi/loader/loader_efi.h               |  2 +
> 5 files changed, 180 insertions(+), 11 deletions(-)
>
>diff --git a/stand/efi/loader/arch/amd64/amd64_tramp.S b/stand/efi/loader/arch/amd64/amd64_tramp.S
>index 877705407f9..07354308e73 100644
>--- a/stand/efi/loader/arch/amd64/amd64_tramp.S
>+++ b/stand/efi/loader/arch/amd64/amd64_tramp.S
>@@ -61,6 +61,54 @@ amd64_tramp:
> 	ALIGN_TEXT
> amd64_tramp_end:
> 
>+/*
>+ * void amd64_tramp_inline(uint64_t stack, uint64_t kernend,
>+ *			uint64_t modulep, uint64_t pagetable, uint64_t entry,
>+ *			uint64_t copy_dst, uint64_t copy_src, uint64_t copy_src_end)
>+ */
>+	.globl	amd64_tramp_inline
>+amd64_tramp_inline:
>+	cli			/* Make sure we don't get interrupted. */
>+
>+	movq	%rsi,%r12	/* Stash the kernel values for later. */
>+	movq	%rdx,%r13
>+	movq	%rcx,%r14
>+	movq	%r8,%r15
>+
>+	/* Copy the kernel from the staging area to the expected location
>+	 * in memory. The following code is equivalent to the efi_copy_finish
>+	 * function that amd64_tramp used to call. Inlining this code avoids
>+	 * a scenario when the system froze because efi_copy_finish
>+	 * overwrote its own code that just happened to be located somewhere
>+	 * in the destination range.
>+	 *
>+	 * while (copy_src < copy_src_end) *copy_dst++ = *copy_src++;
>+	 */
>+	movq	8(%rsp), %rax	/* rax = copy_src */
>+	movq	16(%rsp), %rcx	/* rcx = copy_src_end */
>+	cmpq	%rcx, %rax
>+	jnb	copy_done
>+	subq	%rax, %r9		/* r9 = copy_dst - copy_src */
>+loop:
>+	movq	(%rax), %rdx
>+	movq	%rdx, (%rax,%r9)
>+	addq	$8, %rax
>+	cmpq	%rax, %rcx
>+	ja	loop
>+copy_done:
>+
>+	movq	%rdi,%rsp	/* Switch to our temporary stack. */
>+
>+	pushq	%r12		/* Push kernend. */
>+	salq	$32,%r13	/* Shift modulep and push it. */
>+	pushq	%r13
>+	pushq	%r15		/* Push the entry address. */
>+	movq	%r14,%cr3	/* Switch page tables. */
>+	ret			/* "Return" to kernel entry. */
>+
>+	ALIGN_TEXT
>+amd64_tramp_inline_end:
>+
> /* void multiboot2_exec(uint64_t entry, uint64_t multiboot_info, uint64_t stack) */
> 	.globl	multiboot2_exec
> multiboot2_exec:
>@@ -74,3 +122,7 @@ multiboot2_exec:
> 	.globl	amd64_tramp_size
> amd64_tramp_size:
> 	.long	amd64_tramp_end-amd64_tramp
>+
>+	.globl	amd64_tramp_inline_size
>+amd64_tramp_inline_size:
>+	.long	amd64_tramp_inline_end-amd64_tramp_inline
>diff --git a/stand/efi/loader/arch/amd64/elf64_freebsd.c b/stand/efi/loader/arch/amd64/elf64_freebsd.c
>index a950ca55e84..62e99ac63e6 100644
>--- a/stand/efi/loader/arch/amd64/elf64_freebsd.c
>+++ b/stand/efi/loader/arch/amd64/elf64_freebsd.c
>@@ -84,11 +84,12 @@ static pml4_entry_t *PT4;
> static pdp_entry_t *PT3;
> static pd_entry_t *PT2;
> 
>-static void (*trampoline)(uint64_t stack, void *copy_finish, uint64_t kernend,
>-    uint64_t modulep, pml4_entry_t *pagetable, uint64_t entry);
>+static void (*trampoline)(uint64_t stack, uint64_t kernend,
>+	uint64_t modulep, pml4_entry_t *pagetable, uint64_t entry,
>+	uint64_t copy_dst, uint64_t copy_src, uint64_t copy_src_end);
> 
>-extern uintptr_t amd64_tramp;
>-extern uint32_t amd64_tramp_size;
>+extern uintptr_t amd64_tramp_inline;
>+extern uint32_t amd64_tramp_inline_size;
> 
> /*
>  * There is an ELF kernel and one or more ELF modules loaded.
>@@ -101,6 +102,8 @@ elf64_exec(struct preloaded_file *fp)
> 	struct file_metadata	*md;
> 	Elf_Ehdr 		*ehdr;
> 	vm_offset_t		modulep, kernend, trampcode, trampstack;
>+	uint64_t		copy_dst, copy_src, copy_src_end;
>+	EFI_STATUS		status;
> 	int			err, i;
> 	ACPI_TABLE_RSDP		*rsdp;
> 	char			buf[24];
>@@ -155,16 +158,26 @@ elf64_exec(struct preloaded_file *fp)
> 	ehdr = (Elf_Ehdr *)&(md->md_data);
> 
> 	trampcode = (vm_offset_t)0x0000000040000000;
>-	err = BS->AllocatePages(AllocateMaxAddress, EfiLoaderData, 1,
>+	status = BS->AllocatePages(AllocateMaxAddress, EfiLoaderData, 1,
> 	    (EFI_PHYSICAL_ADDRESS *)&trampcode);
>+	if (EFI_ERROR(status)) {
>+		printf("Failed to allocate pages for trampoline code: %lu\n",
>+		    EFI_ERROR_CODE(status));
>+		return(ENOMEM);
>+	}
> 	bzero((void *)trampcode, EFI_PAGE_SIZE);
> 	trampstack = trampcode + EFI_PAGE_SIZE - 8;
>-	bcopy((void *)&amd64_tramp, (void *)trampcode, amd64_tramp_size);
>+	bcopy((void *)&amd64_tramp_inline, (void *)trampcode, amd64_tramp_inline_size);
> 	trampoline = (void *)trampcode;
> 
> 	PT4 = (pml4_entry_t *)0x0000000040000000;
>-	err = BS->AllocatePages(AllocateMaxAddress, EfiLoaderData, 3,
>+	status = BS->AllocatePages(AllocateMaxAddress, EfiLoaderData, 3,
> 	    (EFI_PHYSICAL_ADDRESS *)&PT4);
>+	if (EFI_ERROR(status)) {
>+		printf("Failed to allocate pages for PT4: %lu\n",
>+		    EFI_ERROR_CODE(status));
>+		return(ENOMEM);
>+	}
> 	bzero(PT4, 3 * EFI_PAGE_SIZE);
> 
> 	PT3 = &PT4[512];
>@@ -191,6 +204,15 @@ elf64_exec(struct preloaded_file *fp)
> 
> 	printf("Start @ 0x%lx ...\n", ehdr->e_entry);
> 
>+	/* Check the type of memory pages that will be overwritten
>+	 * by the trampoline and print a warning message for easier
>+	 * debugging. The memory map will most likely change until
>+	 * then, but I don't expect new reserved memory blocks to
>+	 * suddenly appear. */
>+	if (!efi_verify_destination_type()) {
>+		printf("Important memory pages may get overwritten!\n");
>+	}
>+
> 	efi_time_fini();
> 	err = bi_load(fp->f_args, &modulep, &kernend, true);
> 	if (err != 0) {
>@@ -200,8 +222,10 @@ elf64_exec(struct preloaded_file *fp)
> 
> 	dev_cleanup();
> 
>-	trampoline(trampstack, efi_copy_finish, kernend, modulep, PT4,
>-	    ehdr->e_entry);
>+	efi_copy_get_locations(&copy_dst, &copy_src, &copy_src_end);
>+
>+	trampoline(trampstack, kernend, modulep, PT4,
>+	    ehdr->e_entry, copy_dst, copy_src, copy_src_end);
> 
> 	panic("exec returned");
> }
>diff --git a/stand/efi/loader/bootinfo.c b/stand/efi/loader/bootinfo.c
>index 9924901d29e..51fa619a1ea 100644
>--- a/stand/efi/loader/bootinfo.c
>+++ b/stand/efi/loader/bootinfo.c
>@@ -444,6 +444,7 @@ bi_load(char *args, vm_offset_t *modulep, vm_offset_t *kernendp, bool exit_bs)
> 	vm_offset_t size;
> 	char *rootdevname;
> 	int howto;
>+	int err;
> #if defined(LOADER_FDT_SUPPORT)
> 	vm_offset_t dtbp;
> 	int dtb_size;
>@@ -539,7 +540,10 @@ bi_load(char *args, vm_offset_t *modulep, vm_offset_t *kernendp, bool exit_bs)
> #ifdef LOADER_GELI_SUPPORT
> 	geli_export_key_metadata(kfp);
> #endif
>-	bi_load_efi_data(kfp, exit_bs);
>+	err = bi_load_efi_data(kfp, exit_bs);
>+	if (err != 0) {
>+		return(err);
>+	}
> 
> 	size = bi_copymodules(0);
> 	kernend = roundup(addr + size, PAGE_SIZE);
>diff --git a/stand/efi/loader/copy.c b/stand/efi/loader/copy.c
>index e723b61e3bc..5a3834a1983 100644
>--- a/stand/efi/loader/copy.c
>+++ b/stand/efi/loader/copy.c
>@@ -208,7 +208,7 @@ efi_copy_init(void)
> 	 * memory: see elf64_exec() in
> 	 * boot/efi/loader/arch/amd64/elf64_freebsd.c.
> 	 */
>-	staging = 1024*1024*1024;
>+	staging = 1024*1024*1024 - 1;
> 	status = BS->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> 	    nr_pages, &staging);
> #else
>@@ -316,6 +316,9 @@ efi_copyin(const void *src, vm_offset_t dest, const size_t len)
> 	/* XXX: Callers do not check for failure. */
> 	if (!efi_check_space(dest + stage_offset + len)) {
> 		errno = ENOMEM;
>+		/* XXX: For now, it is better to stop
>+		 * booting instead of ignoring it. */
>+		panic("efi_copyin: Staging area is too small");
> 		return (-1);
> 	}
> 	bcopy(src, (void *)(dest + stage_offset), len);
>@@ -329,6 +332,9 @@ efi_copyout(const vm_offset_t src, void *dest, const size_t len)
> 	/* XXX: Callers do not check for failure. */
> 	if (src + stage_offset + len > staging_end) {
> 		errno = ENOMEM;
>+		/* XXX: For now, it is better to stop
>+		 * booting instead of ignoring it. */
>+		panic("efi_copyout: Staging area is too small");
> 		return (-1);
> 	}
> 	bcopy((void *)(src + stage_offset), dest, len);
>@@ -345,8 +351,13 @@ efi_readin(readin_handle_t fd, vm_offset_t dest, const size_t len)
> 		stage_offset_set = 1;
> 	}
> 
>+	/* XXX: Callers don't seem to differentiate between a read failure
>+	 * from fd and an insufficient space in the staging area. */
> 	if (!efi_check_space(dest + stage_offset + len)) {
> 		errno = ENOMEM;
>+		/* XXX: For now, at least print that the size of
>+		 * the staging area is at fault and should be larger. */
>+		printf("efi_readin: Staging area is too small\n");
> 		return (-1);
> 	}
> 	return (VECTX_READ(fd, (void *)(dest + stage_offset), len));
>@@ -364,3 +375,79 @@ efi_copy_finish(void)
> 	while (src < last)
> 		*dst++ = *src++;
> }
>+
>+void
>+efi_copy_get_locations(uint64_t *dst, uint64_t *src, uint64_t *src_end)
>+{
>+	*src = (uint64_t)staging;
>+	*dst = (uint64_t)(staging - stage_offset);
>+	*src_end = (uint64_t)staging_end;
>+}
>+
>+bool
>+efi_verify_destination_type(void)
>+{
>+	EFI_MEMORY_DESCRIPTOR *map = NULL, *p;
>+	EFI_PHYSICAL_ADDRESS dest, dest_end;
>+	EFI_PHYSICAL_ADDRESS page_start, page_end, overlap_start, overlap_end;
>+	UINTN sz, key, dsz;
>+	UINT32 dver;
>+	EFI_STATUS status;
>+	int i, ndesc;
>+	bool safe;
>+
>+	dest = staging - stage_offset;
>+	dest_end = dest + (staging_end - staging);
>+	safe = false;
>+
>+	sz = 0;
>+
>+	for (;;) {
>+		status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver);
>+		if (!EFI_ERROR(status))
>+			break;
>+
>+		if (status != EFI_BUFFER_TOO_SMALL) {
>+			printf("Can't read memory map: %lu\n",
>+			    EFI_ERROR_CODE(status));
>+			goto out;
>+		}
>+
>+		free(map);
>+
>+		/* Allocate 10 descriptors more than the size reported,
>+		 * to allow for any fragmentation caused by calling
>+		 * malloc */
>+		map = malloc(sz + (10 * dsz));
>+		if (map == NULL) {
>+			printf("Unable to allocate memory\n");
>+			goto out;
>+		}
>+	}
>+
>+	safe = true;
>+
>+	ndesc = sz / dsz;
>+	for (i = 0, p = map; i < ndesc;
>+	     i++, p = NextMemoryDescriptor(p, dsz)) {
>+		page_start = p->PhysicalStart;
>+		page_end = page_start + p->NumberOfPages * EFI_PAGE_SIZE;
>+
>+		overlap_start = page_start > dest ? page_start : dest;
>+		overlap_end = page_end < dest_end ? page_end : dest_end;
>+
>+		if (overlap_start < overlap_end &&
>+		    p->Type != EfiLoaderCode &&
>+		    p->Type != EfiLoaderData &&
>+		    p->Type != EfiBootServicesCode &&
>+		    p->Type != EfiBootServicesData &&
>+		    p->Type != EfiConventionalMemory) {
>+			safe = false;
>+			break;
>+		}
>+	}
>+
>+out:
>+	free(map);
>+	return safe;
>+}
>\ No newline at end of file
>diff --git a/stand/efi/loader/loader_efi.h b/stand/efi/loader/loader_efi.h
>index 4d077514e42..f2e65dc95b4 100644
>--- a/stand/efi/loader/loader_efi.h
>+++ b/stand/efi/loader/loader_efi.h
>@@ -44,5 +44,7 @@ ssize_t	efi_readin(readin_handle_t fd, vm_offset_t dest, const size_t len);
> void * efi_translate(vm_offset_t ptr);
> 
> void	efi_copy_finish(void);
>+void efi_copy_get_locations(uint64_t *dst, uint64_t *src, uint64_t *src_end);
>+bool efi_verify_destination_type(void);
> 
> #endif	/* _LOADER_EFI_COPY_H_ */
>-- 
>2.31.1
>

Actions: View | Diff

Attachments on bug 209821: 225333 | 225480 | 230798 | 230812 | 230823