linux-kernel - [RFC PATCH] x86: efi: Reserve all boot services regions to avoid memory reservation conflicts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20240617111153.1945652-1-lihuafei1@huawei.com>
Date: Mon, 17 Jun 2024 19:11:53 +0800
From: Li Huafei <lihuafei1@...wei.com>
To: <linux-efi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <ardb@...nel.org>, <bhe@...hat.com>, <dyoung@...hat.com>,
	<chenhaixiang3@...wei.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <hpa@...or.com>,
	<yeweihua4@...wei.com>, <kexec@...ts.infradead.org>, <x86@...nel.org>
Subject: [RFC PATCH] x86: efi: Reserve all boot services regions to avoid memory reservation conflicts

We encountered a conflict between kdump crashkernel reserved memory and
bgrt reserved image_address memory:

  # cat /proc/iomem
  ...
  2d4fd058-60efefff : System RAM
    2d4fd058-58ffffff : System RAM
      49000000-58ffffff : Crash kernel
        53cbd000-53ccffff : Reserved <== Rrserved by bgrt
  ...

This resulted in the kexec tool generating incorrect e820 tables and the
kdump kernel failed to boot. After analysis, the reason for the memory
reservation conflict is that the mem_attr_table memory falls into an
EFI_BOOT_SERVICES_DATA region, and efi_memattr_init() reserved some of
the memory in this region, efi_reserve_boot_services() not fully
reserving this memory region. The bgrt image_address memory happens to
be in this region as well, thus the memory for bgrt image_address fails
to be reserved in advance. Full details are in [1].

To resolve conflicts, we may need to reserve all EFI_BOOT_SERVICES_DATA
memory in efi_reserve_boot_services(), even if there is an overlap with
other reserved areas.

[1] https://lore.kernel.org/all/ZfjshNK7Nbdz1de3@MiWiFi-R3L-srv/T/#me0e8a206833d1e6caf962b9e2c0c204bf966c769

Signed-off-by: Li Huafei <lihuafei1@...wei.com>
---
 arch/x86/platform/efi/quirks.c | 35 ++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index f0cc00032751..02863f0305db 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -337,10 +337,26 @@ void __init efi_reserve_boot_services(void)
 		    md->type != EFI_BOOT_SERVICES_DATA)
 			continue;
 
+		/*
+		 * Checks if the current region has been partially or fully
+		 * reserved by someone else.
+		 */
 		already_reserved = memblock_is_region_reserved(start, size);
 
 		/*
-		 * Because the following memblock_reserve() is paired
+		 * If someone else has already reserved part of the EFI Boot
+		 * Data region (e.g. efi_memattr_init()), we should also
+		 * reserve the remaining part. This is because some efi drivers
+		 * rely on reserving EFI Boot Data here in advance, such as
+		 * bgrt (see efi_bgrt_init()), otherwise the memory reserved
+		 * by bgrt via efi_mem_reserve() will conflict with the memory
+		 * reserved by others via memblock_reserve(). So here
+		 * unconditionally reserve the whole EFI Boot Data region.
+		 */
+		memblock_reserve(start, size);
+
+		/*
+		 * Because the above memblock_reserve() is paired
 		 * with memblock_free_late() for this region in
 		 * efi_free_boot_services(), we must be extremely
 		 * careful not to reserve, and subsequently free,
@@ -352,18 +368,13 @@ void __init efi_reserve_boot_services(void)
 		 * freed is page zero (first 4Kb of memory), which may
 		 * contain boot services code/data but is marked
 		 * E820_TYPE_RESERVED by trim_bios_range().
+		 *
+		 * If we are the first to reserve the region, no
+		 * one else cares about it. We own it and can
+		 * free it later.
 		 */
-		if (!already_reserved) {
-			memblock_reserve(start, size);
-
-			/*
-			 * If we are the first to reserve the region, no
-			 * one else cares about it. We own it and can
-			 * free it later.
-			 */
-			if (can_free_region(start, size))
-				continue;
-		}
+		if (!already_reserved && can_free_region(start, size))
+			continue;
 
 		/*
 		 * We don't own the region. We must not free it.
-- 
2.25.1