[PATCH 1/3] kernel-stub: Copy the kernel out of its section into a buffer

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

This is unavoidable as the UEFI firmware may decide to enforce the
section settings, making the region either executable (which it wasn't
so far) or writable, but not both at the same time. Seen with EDK2 on
arm64 so far, interestingly not on x86 yet. U-Boot didn't care at all.

Another issue of the current approach is that transplanting the kernel's
SectionAlignment into the unified image may also cause shifting of the
stub's sections if their original virtual address does not fulfill that,
and that shifting can break their relationship.

So this adopts the allocate and copy-out strategy of systemd-boot and
cleans up the generator script accordingly.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
kernel-stub/main.c | 55 ++++++++++++++++++++++++++++++++-----
tools/bg_gen_unified_kernel | 4 +--
2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index 95a9593..c9517a5 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -34,7 +34,11 @@ typedef struct {
typedef struct {
UINT8 Ignore1[16];
UINT32 AddressOfEntryPoint;
- UINT8 Ignore2[220];
+ UINT8 Ignore2[12];
+ UINT32 SectionAlignment;
+ UINT8 Ignore3[20];
+ UINT32 SizeOfImage;
+ UINT8 Ignore4[180];
} __attribute__((packed)) OPT_HEADER;

typedef struct {
@@ -53,6 +57,12 @@ typedef struct {
static EFI_HANDLE this_image;
static EFI_LOADED_IMAGE kernel_image;

+EFI_PHYSICAL_ADDRESS align_addr(EFI_PHYSICAL_ADDRESS ptr,
+ EFI_PHYSICAL_ADDRESS align)
+{
+ return (ptr + align - 1) & ~(align - 1);
+}
+
VOID __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
{
Print(L"Unified kernel stub: %s (%r).\n", message, status);
@@ -87,6 +97,8 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
const SECTION *initrd_section = NULL;
EFI_HANDLE kernel_handle = NULL;
BOOLEAN has_dtbs = FALSE;
+ const VOID *kernel_source;
+ EFI_PHYSICAL_ADDRESS kernel_buffer;
const CHAR8 *fdt_compatible;
VOID *fdt, *alt_fdt = NULL;
EFI_IMAGE_ENTRY_POINT kernel_entry;
@@ -94,7 +106,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
const PE_HEADER *pe_header;
const SECTION *section;
EFI_STATUS status, kernel_status;
- UINTN n;
+ UINTN n, kernel_pages;

this_image = image_handle;
InitializeLib(image_handle, system_table);
@@ -140,10 +152,6 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
error_exit(L"Missing .kernel section", EFI_NOT_FOUND);
}

- kernel_image.ImageBase = (UINT8 *) stub_image->ImageBase +
- kernel_section->VirtualAddress;
- kernel_image.ImageSize = kernel_section->VirtualSize;
-
if (cmdline_section) {
kernel_image.LoadOptions = (UINT8 *) stub_image->ImageBase +
cmdline_section->VirtualAddress;
@@ -157,10 +165,43 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
initrd_section->VirtualSize);
}

+ /*
+ * Allocate new home for the kernel image. This is needed because
+ * - its section is either not executable or not writable
+ * - section alignment in virtual memory may not fit
+ *
+ * The new buffer size is based from SizeOfImage, aligned according to
+ * the kernels SectionAlignment. As SectionAlignment may be larger than
+ * the page size, over-allocate in order to adjust the base as needed.
+ */
+ kernel_source = (UINT8 *) stub_image->ImageBase +
+ kernel_section->VirtualAddress;
+
+ pe_header = get_pe_header(kernel_source);
+
+ kernel_pages = EFI_SIZE_TO_PAGES(pe_header->Opt.SizeOfImage +
+ pe_header->Opt.SectionAlignment);
+ status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData,
+ kernel_pages, &kernel_buffer);
+ if (EFI_ERROR(status)) {
+ uninstall_initrd_loader();
+ error_exit(L"Error allocating memory for kernel image", status);
+ }
+
+ kernel_image.ImageBase = (VOID *)
+ align_addr(kernel_buffer, pe_header->Opt.SectionAlignment);
+ kernel_image.ImageSize = kernel_section->VirtualSize;
+
+ CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
+ /* Clear the rest so that .bss is definitely zero. */
+ SetMem((UINT8 *) kernel_image.ImageBase + kernel_image.ImageSize,
+ pe_header->Opt.SizeOfImage - kernel_image.ImageSize, 0);
+
status = BS->InstallMultipleProtocolInterfaces(
&kernel_handle, &LoadedImageProtocol, &kernel_image,
NULL);
if (EFI_ERROR(status)) {
+ BS->FreePages(kernel_buffer, kernel_pages);
uninstall_initrd_loader();
error_exit(L"Error registering kernel image", status);
}
@@ -175,7 +216,6 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
info(L"Using firmware-provided device tree");
}

- pe_header = get_pe_header(kernel_image.ImageBase);
kernel_entry = (EFI_IMAGE_ENTRY_POINT)
((UINT8 *) kernel_image.ImageBase +
pe_header->Opt.AddressOfEntryPoint);
@@ -188,6 +228,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
if (EFI_ERROR(status)) {
error_exit(L"Error unregistering kernel image", status);
}
+ BS->FreePages(kernel_buffer, kernel_pages);
uninstall_initrd_loader();

return kernel_status;
diff --git a/tools/bg_gen_unified_kernel b/tools/bg_gen_unified_kernel
index 7cedd82..551bda6 100755
--- a/tools/bg_gen_unified_kernel
+++ b/tools/bg_gen_unified_kernel
@@ -251,13 +251,11 @@ def main():

current_offs = cmdline_section.data_offs + cmdline_section.data_size
sect_size = align(len(kernel), file_align)
- virt_size = max(sect_size, kernel_pe_headers.get_size_of_image())
- kernel_section = Section(b'.kernel', virt_size, 0x2000000,
+ kernel_section = Section(b'.kernel', sect_size, 0x2000000,
sect_size, current_offs,
Section.IMAGE_SCN_CNT_INITIALIZED_DATA |
Section.IMAGE_SCN_MEM_READ)
pe_headers.add_section(kernel_section)
- pe_headers.set_section_alignment(kernel_pe_headers.get_section_alignment())

current_offs = kernel_section.data_offs + kernel_section.data_size
if args.initrd:
--
2.35.3

[Christian Storm]

Hi,

Shouldn't this read '[based] on'?

Here, you're not freeing kernel_buffer and not uninstalling the initrd
loader.

> error_exit(L"Error unregistering kernel image", status);
> }
> + BS->FreePages(kernel_buffer, kernel_pages);
> uninstall_initrd_loader();
>
> return kernel_status;
> diff --git a/tools/bg_gen_unified_kernel b/tools/bg_gen_unified_kernel
> index 7cedd82..551bda6 100755
> --- a/tools/bg_gen_unified_kernel
> +++ b/tools/bg_gen_unified_kernel
> @@ -251,13 +251,11 @@ def main():
>
> current_offs = cmdline_section.data_offs + cmdline_section.data_size
> sect_size = align(len(kernel), file_align)
> - virt_size = max(sect_size, kernel_pe_headers.get_size_of_image())
> - kernel_section = Section(b'.kernel', virt_size, 0x2000000,
> + kernel_section = Section(b'.kernel', sect_size, 0x2000000,
> sect_size, current_offs,
> Section.IMAGE_SCN_CNT_INITIALIZED_DATA |
> Section.IMAGE_SCN_MEM_READ)
> pe_headers.add_section(kernel_section)
> - pe_headers.set_section_alignment(kernel_pe_headers.get_section_alignment())
>
> current_offs = kernel_section.data_offs + kernel_section.data_size
> if args.initrd:
> --
> 2.35.3
>

Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

This is unavoidable as the UEFI firmware may decide to enforce the
section settings, making the region either executable (which it wasn't
so far) or writable, but not both at the same time. Seen with EDK2 on
arm64 so far, interestingly not on x86 yet. U-Boot didn't care at all.

Another issue of the current approach is that transplanting the kernel's
SectionAlignment into the unified image may also cause shifting of the
stub's sections if their original virtual address does not fulfill that,
and that shifting can break their relationship.

So this adopts the allocate and copy-out strategy of systemd-boot and
cleans up the generator script accordingly.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

kernel-stub/main.c | 57 +++++++++++++++++++++++++++++++------
tools/bg_gen_unified_kernel | 5 +---
2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index 3858b24..5cf06ac 100644

--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -34,7 +34,11 @@ typedef struct {
typedef struct {
UINT8 Ignore1[16];
UINT32 AddressOfEntryPoint;
- UINT8 Ignore2[220];
+ UINT8 Ignore2[12];
+ UINT32 SectionAlignment;
+ UINT8 Ignore3[20];
+ UINT32 SizeOfImage;
+ UINT8 Ignore4[180];
} __attribute__((packed)) OPT_HEADER;

typedef struct {
@@ -53,6 +57,12 @@ typedef struct {
static EFI_HANDLE this_image;
static EFI_LOADED_IMAGE kernel_image;

+EFI_PHYSICAL_ADDRESS align_addr(EFI_PHYSICAL_ADDRESS ptr,
+ EFI_PHYSICAL_ADDRESS align)
+{
+ return (ptr + align - 1) & ~(align - 1);
+}
+

static VOID info(CHAR16 *message)
{
Print(L"Unified kernel stub: %s\n", message);
@@ -92,6 +102,8 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

const SECTION *initrd_section = NULL;
EFI_HANDLE kernel_handle = NULL;
BOOLEAN has_dtbs = FALSE;
+ const VOID *kernel_source;
+ EFI_PHYSICAL_ADDRESS kernel_buffer;
const CHAR8 *fdt_compatible;
VOID *fdt, *alt_fdt = NULL;
EFI_IMAGE_ENTRY_POINT kernel_entry;

@@ -99,7 +111,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

const PE_HEADER *pe_header;
const SECTION *section;

EFI_STATUS status, cleanup_status;

- UINTN n;
+ UINTN n, kernel_pages;

this_image = image_handle;
InitializeLib(image_handle, system_table);

@@ -145,10 +157,6 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

error_exit(L"Missing .kernel section", EFI_NOT_FOUND);
}

- kernel_image.ImageBase = (UINT8 *) stub_image->ImageBase +
- kernel_section->VirtualAddress;
- kernel_image.ImageSize = kernel_section->VirtualSize;
-
if (cmdline_section) {
kernel_image.LoadOptions = (UINT8 *) stub_image->ImageBase +
cmdline_section->VirtualAddress;

@@ -162,12 +170,44 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

initrd_section->VirtualSize);
}

+ /*
+ * Allocate new home for the kernel image. This is needed because
+ * - its section is either not executable or not writable
+ * - section alignment in virtual memory may not fit
+ *
+ * The new buffer size is based from SizeOfImage, aligned according to

+ * the kernels SectionAlignment. As SectionAlignment may be larger than
+ * the page size, over-allocate in order to adjust the base as needed.
+ */
+ kernel_source = (UINT8 *) stub_image->ImageBase +
+ kernel_section->VirtualAddress;
+
+ pe_header = get_pe_header(kernel_source);
+
+ kernel_pages = EFI_SIZE_TO_PAGES(pe_header->Opt.SizeOfImage +
+ pe_header->Opt.SectionAlignment);
+ status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData,
+ kernel_pages, &kernel_buffer);
+ if (EFI_ERROR(status)) {

+ error(L"Error allocating memory for kernel image", status);
+ goto cleanup_initrd;

+ }
+
+ kernel_image.ImageBase = (VOID *)
+ align_addr(kernel_buffer, pe_header->Opt.SectionAlignment);
+ kernel_image.ImageSize = kernel_section->VirtualSize;
+
+ CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
+ /* Clear the rest so that .bss is definitely zero. */
+ SetMem((UINT8 *) kernel_image.ImageBase + kernel_image.ImageSize,
+ pe_header->Opt.SizeOfImage - kernel_image.ImageSize, 0);
+
status = BS->InstallMultipleProtocolInterfaces(
&kernel_handle, &LoadedImageProtocol, &kernel_image,
NULL);
if (EFI_ERROR(status)) {

error(L"Error registering kernel image", status);
- goto cleanup_initrd;
+ goto cleanup_buffer;
}

if (alt_fdt) {
@@ -183,7 +223,6 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

info(L"Using firmware-provided device tree");
}

- pe_header = get_pe_header(kernel_image.ImageBase);
kernel_entry = (EFI_IMAGE_ENTRY_POINT)
((UINT8 *) kernel_image.ImageBase +
pe_header->Opt.AddressOfEntryPoint);

@@ -200,6 +239,8 @@ cleanup_protocols:
status = cleanup_status;
}
}
+cleanup_buffer:
+ BS->FreePages(kernel_buffer, kernel_pages);
cleanup_initrd:
uninstall_initrd_loader();

diff --git a/tools/bg_gen_unified_kernel b/tools/bg_gen_unified_kernel
index 7cedd82..dc5328c 100755
--- a/tools/bg_gen_unified_kernel
+++ b/tools/bg_gen_unified_kernel
@@ -247,17 +247,14 @@ def main():
pe_headers.add_section(cmdline_section)

kernel = args.kernel.read()
- kernel_pe_headers = PEHeaders('kernel', kernel)