by tip-bot2 for Hou Wenlong

[permalink] [raw]

Subject: [tip: efi/urgent] efi/x86: Don't remap text<->rodata gap read-only for mixed mode

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: f6103162008dfd37567f240b50e5e1ea7cf2e00c
Gitweb: https://git.kernel.org/tip/f6103162008dfd37567f240b50e5e1ea7cf2e00c
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 09 Apr 2020 15:04:34 +02:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:17 +02:00

efi/x86: Don't remap text<->rodata gap read-only for mixed mode

Commit

d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

updated the code that creates the 1:1 memory mapping to use read-only
attributes for the 1:1 alias of the kernel's text and rodata sections, to
protect it from inadvertent modification. However, it failed to take into
account that the unused gap between text and rodata is given to the page
allocator for general use.

If the vmap'ed stack happens to be allocated from this region, any by-ref
output arguments passed to EFI runtime services that are allocated on the
stack (such as the 'datasize' argument taken by GetVariable() when invoked
from efivar_entry_size()) will be referenced via a read-only mapping,
resulting in a page fault if the EFI code tries to write to it:

BUG: unable to handle page fault for address: 00000000386aae88
#PF: supervisor write access in kernel mode
#PF: error_code(0x0003) - permissions violation
PGD fd61063 P4D fd61063 PUD fd62063 PMD 386000e1
Oops: 0003 [#1] SMP PTI
CPU: 2 PID: 255 Comm: systemd-sysv-ge Not tainted 5.6.0-rc4-default+ #22
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
RIP: 0008:0x3eaeed95
Code: ... <89> 03 be 05 00 00 80 a1 74 63 b1 3e 83 c0 48 e8 44 d2 ff ff eb 05
RSP: 0018:000000000fd73fa0 EFLAGS: 00010002
RAX: 0000000000000001 RBX: 00000000386aae88 RCX: 000000003e9f1120
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
RBP: 000000000fd73fd8 R08: 00000000386aae88 R09: 0000000000000000
R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
R13: ffffc0f040220000 R14: 0000000000000000 R15: 0000000000000000
FS: 00007f21160ac940(0000) GS:ffff9cf23d500000(0000) knlGS:0000000000000000
CS: 0008 DS: 0018 ES: 0018 CR0: 0000000080050033
CR2: 00000000386aae88 CR3: 000000000fd6c004 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
Modules linked in:
CR2: 00000000386aae88
---[ end trace a8bfbd202e712834 ]---

Let's fix this by remapping text and rodata individually, and leave the
gaps mapped read-write.

Fixes: d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")
Reported-by: Jiri Slaby <[email protected]>
Tested-by: Jiri Slaby <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/platform/efi/efi_64.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e0e2e81..c5e393f 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -202,7 +202,7 @@ virt_to_phys_or_null_size(void *va, unsigned long size)

int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
{
- unsigned long pfn, text, pf;
+ unsigned long pfn, text, pf, rodata;
struct page *page;
unsigned npages;
pgd_t *pgd = efi_mm.pgd;
@@ -256,7 +256,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)

efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */

- npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
+ npages = (_etext - _text) >> PAGE_SHIFT;
text = __pa(_text);
pfn = text >> PAGE_SHIFT;

@@ -266,6 +266,14 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
return 1;
}

+ npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
+ rodata = __pa(__start_rodata);
+ pfn = rodata >> PAGE_SHIFT;
+ if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
+ pr_err("Failed to map kernel rodata 1:1\n");
+ return 1;
+ }
+
return 0;
}

2020-04-15 19:08:23

by tip-bot2 for Hou Wenlong

[permalink] [raw]

Subject: [tip: efi/urgent] Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: 8b84769a7a1505b279b337dae83d16390e83f5c1
Gitweb: https://git.kernel.org/tip/8b84769a7a1505b279b337dae83d16390e83f5c1
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 09 Apr 2020 15:04:31 +02:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:15 +02:00

Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

The EFI handover protocol was introduced on x86 to permit the boot
loader to pass a populated boot_params structure as an additional
function argument to the entry point. This allows the bootloader to
pass the base and size of a initrd image, which is more flexible
than relying on the EFI stub's file I/O routines, which can only
access the file system from which the kernel image itself was loaded
from firmware.

This approach requires a fair amount of internal knowledge regarding
the layout of the boot_params structure on the part of the boot loader,
as well as knowledge regarding the allowed placement of the initrd in
memory, and so it has been deprecated in favour of a new initrd loading
method that is based on existing UEFI protocols and best practices.

So update the x86 boot protocol documentation to clarify that the EFI
handover protocol has been deprecated, and while at it, add a note that
invoking the EFI handover protocol still requires the PE/COFF image to
be loaded properly (as opposed to simply being copied into memory).
Also, drop the code32_start header field from the list of values that
need to be provided, as this is no longer required.

Reviewed-by: Borislav Petkov <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
Documentation/x86/boot.rst | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index fa7ddc0..5325c71 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -1399,8 +1399,8 @@ 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.

-EFI Handover Protocol
-=====================
+EFI Handover Protocol (deprecated)
+==================================

This protocol allows boot loaders to defer initialisation to the EFI
boot stub. The boot loader is required to load the kernel/initrd(s)
@@ -1408,6 +1408,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}.

+The boot loader MUST respect the kernel's PE/COFF metadata when it comes
+to section alignment, the memory footprint of the executable image beyond
+the size of the file itself, and any other aspect of the PE/COFF header
+that may affect correct operation of the image as a PE/COFF binary in the
+execution context provided by the EFI firmware.
+
The function prototype for the handover entry point looks like this::

efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp)
@@ -1419,9 +1425,18 @@ UEFI specification. 'bp' is the boot loader-allocated boot params.

The boot loader *must* fill out the following fields in bp::

- - hdr.code32_start
- hdr.cmd_line_ptr
- hdr.ramdisk_image (if applicable)
- hdr.ramdisk_size (if applicable)

All other fields should be zero.
+
+NOTE: The EFI Handover Protocol is deprecated in favour of the ordinary PE/COFF
+ entry point, combined with the LINUX_EFI_INITRD_MEDIA_GUID based initrd
+ loading protocol (refer to [0] for an example of the bootloader side of
+ this), which removes the need for any knowledge on the part of the EFI
+ bootloader regarding the internal representation of boot_params or any
+ requirements/limitations regarding the placement of the command line
+ and ramdisk in memory, or the placement of the kernel image itself.
+
+[0] https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0