From 589c3f289e05454be23507767439cb9769a2264a Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 20 Feb 2025 13:51:29 -0500
Subject: Move memory attribute support to its own file.

This moves the EFI Memory Attribute Protocol helper functions to their
own file, since they're not related to PE things.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 memattrs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)
 create mode 100644 memattrs.c

(limited to 'memattrs.c')

diff --git a/memattrs.c b/memattrs.c
new file mode 100644
index 00000000..99268cd1
--- /dev/null
+++ b/memattrs.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+/*
+ * memattrs.c - EFI and DXE memory attribute helpers
+ * Copyright Peter Jones <pjones@redhat.com>
+ */
+
+#include "shim.h"
+
+static inline uint64_t
+shim_mem_attrs_to_uefi_mem_attrs (uint64_t attrs)
+{
+	uint64_t ret = EFI_MEMORY_RP |
+		       EFI_MEMORY_RO |
+		       EFI_MEMORY_XP;
+
+	if (attrs & MEM_ATTR_R)
+		ret &= ~EFI_MEMORY_RP;
+
+	if (attrs & MEM_ATTR_W)
+		ret &= ~EFI_MEMORY_RO;
+
+	if (attrs & MEM_ATTR_X)
+		ret &= ~EFI_MEMORY_XP;
+
+	return ret;
+}
+
+static inline uint64_t
+uefi_mem_attrs_to_shim_mem_attrs (uint64_t attrs)
+{
+	uint64_t ret = MEM_ATTR_R |
+		       MEM_ATTR_W |
+		       MEM_ATTR_X;
+
+	if (attrs & EFI_MEMORY_RP)
+		ret &= ~MEM_ATTR_R;
+
+	if (attrs & EFI_MEMORY_RO)
+		ret &= ~MEM_ATTR_W;
+
+	if (attrs & EFI_MEMORY_XP)
+		ret &= ~MEM_ATTR_X;
+
+	return ret;
+}
+
+EFI_STATUS
+get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs)
+{
+	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
+	EFI_PHYSICAL_ADDRESS physaddr = addr;
+	EFI_STATUS efi_status;
+
+	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
+				       (VOID **)&proto);
+	if (EFI_ERROR(efi_status) || !proto) {
+		dprint(L"No memory attribute protocol found: %r\n", efi_status);
+		if (!EFI_ERROR(efi_status))
+			efi_status = EFI_UNSUPPORTED;
+		return efi_status;
+	}
+
+	if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0 || attrs == NULL) {
+		dprint(L"%a called on 0x%llx-0x%llx and attrs 0x%llx\n",
+		       __func__, (unsigned long long)physaddr,
+		       (unsigned long long)(physaddr+size-1),
+		       attrs);
+		return EFI_SUCCESS;
+	}
+
+	efi_status = proto->GetMemoryAttributes(proto, physaddr, size, attrs);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"GetMemoryAttributes(..., 0x%llx, 0x%x, 0x%x): %r\n",
+		       physaddr, size, attrs, efi_status);
+	} else {
+		*attrs = uefi_mem_attrs_to_shim_mem_attrs (*attrs);
+	}
+
+	return efi_status;
+}
+
+EFI_STATUS
+update_mem_attrs(uintptr_t addr, uint64_t size,
+		 uint64_t set_attrs, uint64_t clear_attrs)
+{
+	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
+	EFI_PHYSICAL_ADDRESS physaddr = addr;
+	EFI_STATUS efi_status, ret;
+	uint64_t before = 0, after = 0, uefi_set_attrs, uefi_clear_attrs;
+
+	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
+				       (VOID **)&proto);
+	if (EFI_ERROR(efi_status) || !proto)
+		return efi_status;
+
+	efi_status = get_mem_attrs (addr, size, &before);
+	if (EFI_ERROR(efi_status))
+		dprint(L"get_mem_attrs(0x%llx, 0x%llx, 0x%llx) -> 0x%lx\n",
+		       (unsigned long long)addr, (unsigned long long)size,
+		       &before, efi_status);
+
+	if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0) {
+		perror(L"Invalid call %a(addr:0x%llx-0x%llx, size:0x%llx, +%a%a%a, -%a%a%a)\n",
+		       __func__, (unsigned long long)physaddr,
+		       (unsigned long long)(physaddr + size - 1),
+		       (unsigned long long)size,
+		       (set_attrs & MEM_ATTR_R) ? "r" : "",
+		       (set_attrs & MEM_ATTR_W) ? "w" : "",
+		       (set_attrs & MEM_ATTR_X) ? "x" : "",
+		       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+		       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+		       (clear_attrs & MEM_ATTR_X) ? "x" : "");
+		if (!IS_PAGE_ALIGNED(physaddr))
+			perror(L" addr is not page aligned\n");
+		if (!IS_PAGE_ALIGNED(size))
+			perror(L" size is not page aligned\n");
+		if (size == 0)
+			perror(L" size is 0\n");
+		return 0;
+	}
+
+	uefi_set_attrs = shim_mem_attrs_to_uefi_mem_attrs (set_attrs);
+	dprint("translating set_attrs from 0x%lx to 0x%lx\n", set_attrs, uefi_set_attrs);
+	uefi_clear_attrs = shim_mem_attrs_to_uefi_mem_attrs (clear_attrs);
+	dprint("translating clear_attrs from 0x%lx to 0x%lx\n", clear_attrs, uefi_clear_attrs);
+	efi_status = EFI_SUCCESS;
+	if (uefi_set_attrs) {
+		efi_status = proto->SetMemoryAttributes(proto, physaddr, size, uefi_set_attrs);
+		if (EFI_ERROR(efi_status)) {
+			dprint(L"Failed to set memory attrs:0x%0x physaddr:0x%llx size:0x%0lx status:%r\n",
+				uefi_set_attrs, physaddr, size, efi_status);
+		}
+	}
+	if (!EFI_ERROR(efi_status) && uefi_clear_attrs) {
+		efi_status = proto->ClearMemoryAttributes(proto, physaddr, size, uefi_clear_attrs);
+		if (EFI_ERROR(efi_status)) {
+			dprint(L"Failed to clear memory attrs:0x%0x physaddr:0x%llx size:0x%0lx status:%r\n",
+				uefi_clear_attrs, physaddr, size, efi_status);
+		}
+	}
+	ret = efi_status;
+
+	efi_status = get_mem_attrs (addr, size, &after);
+	if (EFI_ERROR(efi_status))
+		dprint(L"get_mem_attrs(0x%llx, %llu, 0x%llx) -> 0x%lx\n",
+		       (unsigned long long)addr, (unsigned long long)size,
+		       &after, efi_status);
+
+	dprint(L"set +%a%a%a -%a%a%a on 0x%llx-0x%llx before:%c%c%c after:%c%c%c\n",
+	       (set_attrs & MEM_ATTR_R) ? "r" : "",
+	       (set_attrs & MEM_ATTR_W) ? "w" : "",
+	       (set_attrs & MEM_ATTR_X) ? "x" : "",
+	       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+	       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+	       (clear_attrs & MEM_ATTR_X) ? "x" : "",
+	       (unsigned long long)addr, (unsigned long long)(addr + size - 1),
+	       (before & MEM_ATTR_R) ? 'r' : '-',
+	       (before & MEM_ATTR_W) ? 'w' : '-',
+	       (before & MEM_ATTR_X) ? 'x' : '-',
+	       (after & MEM_ATTR_R) ? 'r' : '-',
+	       (after & MEM_ATTR_W) ? 'w' : '-',
+	       (after & MEM_ATTR_X) ? 'x' : '-');
+
+	return ret;
+}
+
+// vim:fenc=utf-8:tw=75:noet
-- 
cgit v1.2.3


From 848667d0f3a99401d93c93b3af16b55e3fb28cea Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 15 May 2024 16:13:13 -0400
Subject: shim: add HSIStatus feature

hughsie asked me if I can make shim tell userland what kinds of accesses
are allowed to the heap, stack, and allocations on the running platform,
so that these could be reported up through fwupd's Host Security ID
program (see https://fwupd.github.io/libfwupdplugin/hsi.html ).

This adds a new config-only (i.e. not a UEFI variable) variable
generated during boot, "/sys/firmware/efi/mok-variables/HSIStatus",
which tells us those properties as well as if the EFI Memory Attribute
Protocol is present.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 MokVars.txt                   | 10 ++++++
 globals.c                     |  1 +
 include/memattrs.h            |  2 ++
 include/mok.h                 | 10 ++++++
 include/test-data-efivars-1.h |  6 ++++
 memattrs.c                    | 72 +++++++++++++++++++++++++++++++++++++++++++
 mok.c                         | 46 +++++++++++++++++++++++++++
 shim.c                        |  1 +
 test-mok-mirror.c             | 13 ++++++++
 9 files changed, 161 insertions(+)

(limited to 'memattrs.c')

diff --git a/MokVars.txt b/MokVars.txt
index 71b42c82..e6e68ce4 100644
--- a/MokVars.txt
+++ b/MokVars.txt
@@ -93,3 +93,13 @@ to trust CA keys in the MokList. BS,NV
 
 MokListTrustedRT: A copy of MokListTrusted made available to the kernel
 at runtime. BS,RT
+
+HSIStatus: Status of various security features:
+  heap-is-executable: 0: heap allocations are not executable by default
+                      1: heap allocations are executable
+  stack-is-executable: 0: UEFI stack is not executable
+                       1: UEFI stack is executable
+  ro-sections-are-writable: 0: read-only sections are not writable
+                            1: read-only sections are writable
+  has-memory-attribute-protocol: 0: platform does not provide the EFI Memory Attribute Protocol
+                                 1: platform does provide the EFI Memory Attribute Protocol
diff --git a/globals.c b/globals.c
index d574df16..de64c44c 100644
--- a/globals.c
+++ b/globals.c
@@ -28,6 +28,7 @@ verification_method_t verification_method;
 SHIM_IMAGE_LOADER shim_image_loader_interface;
 
 UINT8 user_insecure_mode;
+UINTN hsi_status = 0;
 UINT8 ignore_db;
 UINT8 trust_mok_list;
 UINT8 mok_policy = 0;
diff --git a/include/memattrs.h b/include/memattrs.h
index 8fefef22..5c40b4cc 100644
--- a/include/memattrs.h
+++ b/include/memattrs.h
@@ -11,5 +11,7 @@ extern EFI_STATUS get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs);
 extern EFI_STATUS update_mem_attrs(uintptr_t addr, uint64_t size,
 				   uint64_t set_attrs, uint64_t clear_attrs);
 
+extern void get_hsi_mem_info(void);
+
 #endif /* !SHIM_MEMATTRS_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/include/mok.h b/include/mok.h
index c37ccba5..e6921e09 100644
--- a/include/mok.h
+++ b/include/mok.h
@@ -125,5 +125,15 @@ struct mok_variable_config_entry {
  */
 #define MOK_POLICY_REQUIRE_NX	1
 
+extern UINTN hsi_status;
+/* heap is executable */
+#define SHIM_HSI_STATUS_HEAPX		0x00000001ULL
+/* stack is executable */
+#define SHIM_HSI_STATUS_STACKX		0x00000002ULL
+/* read-only sections are writable */
+#define SHIM_HSI_STATUS_ROW		0x00000004ULL
+/* platform provides the EFI Memory Attribute Protocol */
+#define SHIM_HSI_STATUS_HASMAP		0x00000008ULL
+
 #endif /* !SHIM_MOK_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/include/test-data-efivars-1.h b/include/test-data-efivars-1.h
index 2831bd23..d97a4d6d 100644
--- a/include/test-data-efivars-1.h
+++ b/include/test-data-efivars-1.h
@@ -106,5 +106,11 @@ static const unsigned char test_data_efivars_1_MokListTrustedRT[] ={
 	0x01
 };
 
+static const unsigned char test_data_efivars_1_HSIStatus[] =
+	"heap-is-executable: 0\n"
+	"stack-is-executable: 0\n"
+	"ro-sections-are-writable: 0\n"
+	"has-memory-attribute-protocol: 0\n";
+
 #endif /* !TEST_DATA_EFIVARS_1_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/memattrs.c b/memattrs.c
index 99268cd1..988459c3 100644
--- a/memattrs.c
+++ b/memattrs.c
@@ -164,4 +164,76 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
 	return ret;
 }
 
+void
+get_hsi_mem_info(void)
+{
+	EFI_STATUS efi_status;
+	uintptr_t addr;
+	uint64_t attrs = 0;
+	uint32_t *tmp_alloc;
+
+	addr = ((uintptr_t)&get_hsi_mem_info) & ~EFI_PAGE_MASK;
+
+	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
+	if (EFI_ERROR(efi_status)) {
+error:
+		/*
+		 * In this case we can't actually tell anything, so assume
+		 * and report the worst case scenario.
+		 */
+		hsi_status = SHIM_HSI_STATUS_HEAPX |
+			     SHIM_HSI_STATUS_STACKX |
+			     SHIM_HSI_STATUS_ROW;
+		dprint(L"Setting HSI to 0x%lx due to error: %r\n", hsi_status, efi_status);
+		return;
+	} else {
+		hsi_status = SHIM_HSI_STATUS_HASMAP;
+		dprint(L"Setting HSI to 0x%lx\n", hsi_status);
+	}
+
+	if (!(hsi_status & SHIM_HSI_STATUS_HASMAP)) {
+		dprint(L"No memory protocol, not testing further\n");
+		return;
+	}
+
+	hsi_status = SHIM_HSI_STATUS_HASMAP;
+	if (attrs & MEM_ATTR_W) {
+		dprint(L"get_hsi_mem_info() is on a writable page: 0x%x->0x%x\n",
+		       hsi_status, hsi_status | SHIM_HSI_STATUS_ROW);
+		hsi_status |= SHIM_HSI_STATUS_ROW;
+	}
+
+	addr = ((uintptr_t)&addr) & ~EFI_PAGE_MASK;
+	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"get_mem_attrs(0x%016llx, 0x%x, &attrs) failed.\n", addr, EFI_PAGE_SIZE);
+		goto error;
+	}
+
+	if (attrs & MEM_ATTR_X) {
+		dprint(L"Stack variable is on an executable page: 0x%x->0x%x\n",
+		       hsi_status, hsi_status | SHIM_HSI_STATUS_STACKX);
+		hsi_status |= SHIM_HSI_STATUS_STACKX;
+	}
+
+	tmp_alloc = AllocatePool(EFI_PAGE_SIZE);
+	if (!tmp_alloc) {
+		dprint(L"Failed to allocate heap variable.\n");
+		goto error;
+	}
+
+	addr = ((uintptr_t)tmp_alloc) & ~EFI_PAGE_MASK;
+	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
+	FreePool(tmp_alloc);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"get_mem_attrs(0x%016llx, 0x%x, &attrs) failed.\n", addr, EFI_PAGE_SIZE);
+		goto error;
+	}
+	if (attrs & MEM_ATTR_X) {
+		dprint(L"Heap variable is on an executable page: 0x%x->0x%x\n",
+		       hsi_status, hsi_status | SHIM_HSI_STATUS_HEAPX);
+		hsi_status |= SHIM_HSI_STATUS_HEAPX;
+	}
+}
+
 // vim:fenc=utf-8:tw=75:noet
diff --git a/mok.c b/mok.c
index 67a798a3..5c7f9a2b 100644
--- a/mok.c
+++ b/mok.c
@@ -34,6 +34,44 @@ static BOOLEAN check_var(CHAR16 *varname)
 		efi_status_;                                                        \
 	})
 
+static UINTN
+format_hsi_status(UINT8 *buf, size_t sz,
+		  struct mok_state_variable *msv UNUSED)
+{
+	const char heapx[] = "heap-is-executable: ";
+	const char stackx[] = "\nstack-is-executable: ";
+	const char row[] = "\nro-sections-are-writable: ";
+	const char hasmap[] = "\nhas-memory-attribute-protocol: ";
+	const char finale[] = "\n";
+	char *pos;
+
+	/*
+	 * sizeof includes the trailing NUL which is where our 0 or 1 value
+	 * fits
+	 */
+	UINTN ret = sizeof(heapx) + sizeof(stackx) +
+		    sizeof(row) + sizeof(hasmap) +
+		    sizeof(finale);
+
+	if (buf == 0 || sz < ret) {
+		return ret;
+	}
+
+	buf[0] = 0;
+	pos = (char *)buf;
+	pos = stpcpy(pos, heapx);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HEAPX) ? "1" : "0");
+	pos = stpcpy(pos, stackx);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_STACKX) ? "1" : "0");
+	pos = stpcpy(pos, row);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_ROW) ? "1" : "0");
+	pos = stpcpy(pos, hasmap);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASMAP) ? "1" : "0");
+	stpcpy(pos, finale);
+
+	return ret;
+}
+
 /*
  * If the OS has set any of these variables we need to drop into MOK and
  * handle them appropriately
@@ -223,6 +261,14 @@ struct mok_state_variable mok_state_variable_data[] = {
 	 .pcr = 14,
 	 .state = &mok_policy,
 	},
+	{.name = L"HSIStatus",
+	 .name8 = "HSIStatus",
+	 .rtname = L"HSIStatus",
+	 .rtname8 = "HSIStatus",
+	 .guid = &SHIM_LOCK_GUID,
+	 .flags = MOK_VARIABLE_CONFIG_ONLY,
+	 .format = format_hsi_status,
+	},
 	{ NULL, }
 };
 size_t n_mok_state_variables = sizeof(mok_state_variable_data) / sizeof(mok_state_variable_data[0]);
diff --git a/shim.c b/shim.c
index fb86589b..6afb35c5 100644
--- a/shim.c
+++ b/shim.c
@@ -2033,6 +2033,7 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 	}
 
 	init_openssl();
+	get_hsi_mem_info();
 
 	efi_status = load_unbundled_trust(global_image_handle);
 	if (EFI_ERROR(efi_status)) {
diff --git a/test-mok-mirror.c b/test-mok-mirror.c
index 316424ad..38b7ed97 100644
--- a/test-mok-mirror.c
+++ b/test-mok-mirror.c
@@ -398,6 +398,11 @@ test_mok_mirror_with_enough_space(void)
 			  EFI_VARIABLE_RUNTIME_ACCESS,
 		 .ops = { NONE, },
 		},
+		{.guid = SHIM_LOCK_GUID,
+		 .name = L"HSIStatus",
+		 .attrs = 0,
+		 .ops = { NONE, },
+		},
 		{.guid = { 0, },
 		 .name = NULL,
 		}
@@ -424,6 +429,10 @@ test_mok_mirror_with_enough_space(void)
 		 .data_size = sizeof(test_data_efivars_1_MokListTrustedRT),
 		 .data = test_data_efivars_1_MokListTrustedRT
 		},
+		{.name = "HSIStatus",
+		 .data_size = sizeof(test_data_efivars_1_HSIStatus),
+		 .data = test_data_efivars_1_HSIStatus
+		},
 		{.name = { 0, },
 		 .data_size = 0,
 		 .data = NULL,
@@ -614,6 +623,10 @@ test_mok_mirror_setvar_out_of_resources(void)
 		 .data_size = sizeof(test_data_efivars_1_MokListTrustedRT),
 		 .data = test_data_efivars_1_MokListTrustedRT
 		},
+		{.name = "HSIStatus",
+		 .data_size = sizeof(test_data_efivars_1_HSIStatus),
+		 .data = test_data_efivars_1_HSIStatus
+		},
 		{.name = { 0, },
 		 .data_size = 0,
 		 .data = NULL,
-- 
cgit v1.2.3


From c41b1f066b9f279b70d933095f277eddbd7c6433 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 27 Jun 2024 15:19:38 -0400
Subject: Add support for DXE memory attribute updates.

This adds DXE implementations of get_mem_attrs() and update_mem_attrs()
for machines that implement DXE but don't yet have the
EFI_MEMORY_ATTRIBUTE_PROTOCOL.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 include/guid.h |  10 ++
 memattrs.c     | 318 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 290 insertions(+), 38 deletions(-)

(limited to 'memattrs.c')

diff --git a/include/guid.h b/include/guid.h
index e32dfc07..26628d1e 100644
--- a/include/guid.h
+++ b/include/guid.h
@@ -3,6 +3,16 @@
 #ifndef SHIM_GUID_H
 #define SHIM_GUID_H
 
+#define LGUID_FMT L"%08x-%04hx-%04hx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
+#define GUID_FMT "%08x-%04hx-%04hx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
+
+#define GUID_ARGS(guid) \
+  ((EFI_GUID)guid).Data1, ((EFI_GUID)guid).Data2, ((EFI_GUID)guid).Data3, \
+  ((EFI_GUID)guid).Data4[1], ((EFI_GUID)guid).Data4[0], \
+  ((EFI_GUID)guid).Data4[2], ((EFI_GUID)guid).Data4[3], \
+  ((EFI_GUID)guid).Data4[4], ((EFI_GUID)guid).Data4[5], \
+  ((EFI_GUID)guid).Data4[6], ((EFI_GUID)guid).Data4[7]
+
 extern EFI_GUID BDS_GUID;
 extern EFI_GUID GV_GUID;
 extern EFI_GUID SIG_DB;
diff --git a/memattrs.c b/memattrs.c
index 988459c3..a2c1777c 100644
--- a/memattrs.c
+++ b/memattrs.c
@@ -44,21 +44,227 @@ uefi_mem_attrs_to_shim_mem_attrs (uint64_t attrs)
 	return ret;
 }
 
-EFI_STATUS
-get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs)
+static void
+get_dxe_services_table(EFI_DXE_SERVICES_TABLE **dstp)
+{
+	static EFI_DXE_SERVICES_TABLE *dst = NULL;
+
+	if (dst == NULL) {
+	dprint(L"Looking for configuration table " LGUID_FMT L"\n", GUID_ARGS(gEfiDxeServicesTableGuid));
+
+		for (UINTN i = 0; i < ST->NumberOfTableEntries; i++) {
+			EFI_CONFIGURATION_TABLE *ct = &ST->ConfigurationTable[i];
+			dprint(L"Testing configuration table " LGUID_FMT L"\n", GUID_ARGS(ct->VendorGuid));
+			if (CompareMem(&ct->VendorGuid, &gEfiDxeServicesTableGuid, sizeof(EFI_GUID)) != 0)
+				continue;
+
+			dst = (EFI_DXE_SERVICES_TABLE *)ct->VendorTable;
+			dprint(L"Looking for DXE Services Signature 0x%16llx, found signature 0x%16llx\n",
+			       EFI_DXE_SERVICES_TABLE_SIGNATURE, dst->Hdr.Signature);
+			if (dst->Hdr.Signature != EFI_DXE_SERVICES_TABLE_SIGNATURE)
+				continue;
+
+			if (!dst->GetMemorySpaceDescriptor || !dst->SetMemorySpaceAttributes) {
+				/*
+				 * purposefully not treating this as an error so that HSIStatus
+				 * can tell us about it later.
+				 */
+				dprint(L"DXE Services lacks Get/SetMemorySpace* functions\n");
+			}
+
+			dprint(L"Setting dxe_services_table to 0x%llx\n", dst);
+			*dstp = dst;
+			return;
+		}
+	} else {
+		*dstp = dst;
+		return;
+	}
+
+	dst = NULL;
+	dprint(L"Couldn't find DXE services\n");
+}
+
+static EFI_STATUS
+dxe_get_mem_attrs(uintptr_t physaddr, size_t size, uint64_t *attrs)
+{
+	EFI_STATUS status;
+	EFI_GCD_MEMORY_SPACE_DESCRIPTOR desc;
+	EFI_PHYSICAL_ADDRESS start, end, next;
+	EFI_DXE_SERVICES_TABLE *dst = NULL;
+
+	get_dxe_services_table(&dst);
+	if (!dst)
+		return EFI_UNSUPPORTED;
+
+	if (!dst->GetMemorySpaceDescriptor || !dst->SetMemorySpaceAttributes)
+		return EFI_UNSUPPORTED;
+
+	if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0 || attrs == NULL) {
+		dprint(L"%a called on 0x%llx-0x%llx and attrs 0x%llx\n",
+		       __func__, (unsigned long long)physaddr,
+		       (unsigned long long)(physaddr+size-1),
+		       attrs);
+		return EFI_SUCCESS;
+	}
+
+	start = ALIGN_DOWN(physaddr, EFI_PAGE_SIZE);
+	end = ALIGN_UP(physaddr + size, EFI_PAGE_SIZE);
+
+	for (; start < end; start = next) {
+		status = dst->GetMemorySpaceDescriptor(start, &desc);
+		if (EFI_ERROR(status)) {
+			dprint(L"GetMemorySpaceDescriptor(0x%llx, ...): %r\n",
+			       start, status);
+			return status;
+		}
+
+		next = desc.BaseAddress + desc.Length;
+
+		if (desc.GcdMemoryType != EFI_GCD_MEMORY_TYPE_SYSTEM_MEMORY)
+			continue;
+
+		*attrs = uefi_mem_attrs_to_shim_mem_attrs(desc.Attributes);
+		return EFI_SUCCESS;
+	}
+
+	return EFI_NOT_FOUND;
+}
+
+static EFI_STATUS
+dxe_update_mem_attrs(uintptr_t addr, size_t size,
+		     uint64_t set_attrs, uint64_t clear_attrs)
+{
+#if 0
+	EFI_STATUS status;
+	EFI_GCD_MEMORY_SPACE_DESCRIPTOR desc;
+	EFI_PHYSICAL_ADDRESS start, end, next;
+	uint64_t before = 0, after = 0, dxe_set_attrs, dxe_clear_attrs;
+#endif
+	EFI_DXE_SERVICES_TABLE *dst = NULL;
+
+	get_dxe_services_table(&dst);
+	if (!dst)
+		return EFI_UNSUPPORTED;
+
+	if (!dst->GetMemorySpaceDescriptor || !dst->SetMemorySpaceAttributes)
+		return EFI_UNSUPPORTED;
+
+	if (!IS_PAGE_ALIGNED(addr) || !IS_PAGE_ALIGNED(size) || size == 0) {
+		perror(L"Invalid call %a(addr:0x%llx-0x%llx, size:0x%llx, +%a%a%a, -%a%a%a)\n",
+		       __func__, (unsigned long long)addr,
+		       (unsigned long long)(addr + size - 1),
+		       (unsigned long long)size,
+		       (set_attrs & MEM_ATTR_R) ? "r" : "",
+		       (set_attrs & MEM_ATTR_W) ? "w" : "",
+		       (set_attrs & MEM_ATTR_X) ? "x" : "",
+		       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+		       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+		       (clear_attrs & MEM_ATTR_X) ? "x" : "");
+		if (!IS_PAGE_ALIGNED(addr))
+			perror(L" addr is not page aligned\n");
+		if (!IS_PAGE_ALIGNED(size))
+			perror(L" size is not page aligned\n");
+		if (size == 0)
+			perror(L" size is 0\n");
+		return EFI_SUCCESS;
+	}
+
+	/*
+	 * We know this only works coincidentally, so nerfing it for now
+	 * until we have a chance to debug more thoroughly on these niche
+	 * systems.
+	 */
+#if 0
+	start = ALIGN_DOWN(addr, EFI_PAGE_SIZE);
+	end = ALIGN_UP(addr + size, EFI_PAGE_SIZE);
+
+	for (; start < end; start = next) {
+		EFI_PHYSICAL_ADDRESS mod_start;
+		UINT64 mod_size;
+
+		status = dst->GetMemorySpaceDescriptor(start, &desc);
+		if (EFI_ERROR(status)) {
+			dprint(L"GetMemorySpaceDescriptor(0x%llx, ...): %r\n",
+			       start, status);
+			return status;
+		}
+
+		next = desc.BaseAddress + desc.Length;
+
+		if (desc.GcdMemoryType != EFI_GCD_MEMORY_TYPE_SYSTEM_MEMORY)
+			continue;
+
+		mod_start = MAX(start, desc.BaseAddress);
+		mod_size = MIN(end, next) - mod_start;
+
+		before = uefi_mem_attrs_to_shim_mem_attrs(desc.Attributes);
+		dxe_set_attrs = shim_mem_attrs_to_uefi_mem_attrs(set_attrs);
+		dprint("translating set_attrs from 0x%lx to 0x%lx\n", set_attrs, dxe_set_attrs);
+		dxe_clear_attrs = shim_mem_attrs_to_uefi_mem_attrs(clear_attrs);
+		dprint("translating clear_attrs from 0x%lx to 0x%lx\n", clear_attrs, dxe_clear_attrs);
+		desc.Attributes |= dxe_set_attrs;
+		desc.Attributes &= ~dxe_clear_attrs;
+		after = uefi_mem_attrs_to_shim_mem_attrs(desc.Attributes);
+
+		status = dst->SetMemorySpaceAttributes(mod_start, mod_size, desc.Attributes);
+		if (EFI_ERROR(status)) {
+			dprint(L"Failed to update memory attrs:0x%0x addr:0x%llx size:0x%0lx status:%r\n",
+				desc.Attributes, mod_start, mod_size, status);
+			return status;
+		}
+
+		break;
+	}
+
+	dprint(L"set +%a%a%a -%a%a%a on 0x%llx-0x%llx before:%c%c%c after:%c%c%c\n",
+	       (set_attrs & MEM_ATTR_R) ? "r" : "",
+	       (set_attrs & MEM_ATTR_W) ? "w" : "",
+	       (set_attrs & MEM_ATTR_X) ? "x" : "",
+	       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+	       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+	       (clear_attrs & MEM_ATTR_X) ? "x" : "",
+	       (unsigned long long)addr, (unsigned long long)(addr + size - 1),
+	       (before & MEM_ATTR_R) ? 'r' : '-',
+	       (before & MEM_ATTR_W) ? 'w' : '-',
+	       (before & MEM_ATTR_X) ? 'x' : '-',
+	       (after & MEM_ATTR_R) ? 'r' : '-',
+	       (after & MEM_ATTR_W) ? 'w' : '-',
+	       (after & MEM_ATTR_X) ? 'x' : '-');
+#endif
+
+	return EFI_SUCCESS;
+}
+
+static void
+get_efi_mem_attr_protocol(EFI_MEMORY_ATTRIBUTE_PROTOCOL **protop)
+{
+	static EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
+	static bool has_mem_access_proto = true;
+
+	if (proto == NULL && has_mem_access_proto == true) {
+		EFI_STATUS efi_status;
+		efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
+					       (VOID **)&proto);
+		if (EFI_ERROR(efi_status) || !proto) {
+			has_mem_access_proto = false;
+			*protop = NULL;
+		}
+	}
+
+	*protop = proto;
+}
+
+static EFI_STATUS
+efi_get_mem_attrs(uintptr_t addr, size_t size, uint64_t *attrs)
 {
 	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
 	EFI_PHYSICAL_ADDRESS physaddr = addr;
 	EFI_STATUS efi_status;
 
-	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
-				       (VOID **)&proto);
-	if (EFI_ERROR(efi_status) || !proto) {
-		dprint(L"No memory attribute protocol found: %r\n", efi_status);
-		if (!EFI_ERROR(efi_status))
-			efi_status = EFI_UNSUPPORTED;
-		return efi_status;
-	}
+	get_efi_mem_attr_protocol(&proto);
+	if (!proto)
+		return EFI_UNSUPPORTED;
 
 	if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0 || attrs == NULL) {
 		dprint(L"%a called on 0x%llx-0x%llx and attrs 0x%llx\n",
@@ -79,23 +285,22 @@ get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs)
 	return efi_status;
 }
 
-EFI_STATUS
-update_mem_attrs(uintptr_t addr, uint64_t size,
-		 uint64_t set_attrs, uint64_t clear_attrs)
+static EFI_STATUS
+efi_update_mem_attrs(uintptr_t addr, uint64_t size,
+		     uint64_t set_attrs, uint64_t clear_attrs)
 {
 	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
 	EFI_PHYSICAL_ADDRESS physaddr = addr;
 	EFI_STATUS efi_status, ret;
 	uint64_t before = 0, after = 0, uefi_set_attrs, uefi_clear_attrs;
 
-	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
-				       (VOID **)&proto);
-	if (EFI_ERROR(efi_status) || !proto)
-		return efi_status;
+	get_efi_mem_attr_protocol(&proto);
+	if (!proto)
+		return EFI_UNSUPPORTED;
 
-	efi_status = get_mem_attrs (addr, size, &before);
+	efi_status = efi_get_mem_attrs(addr, size, &before);
 	if (EFI_ERROR(efi_status))
-		dprint(L"get_mem_attrs(0x%llx, 0x%llx, 0x%llx) -> 0x%lx\n",
+		dprint(L"efi_get_mem_attrs(0x%llx, 0x%llx, 0x%llx) -> 0x%lx\n",
 		       (unsigned long long)addr, (unsigned long long)size,
 		       &before, efi_status);
 
@@ -116,7 +321,7 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
 			perror(L" size is not page aligned\n");
 		if (size == 0)
 			perror(L" size is 0\n");
-		return 0;
+		return EFI_SUCCESS;
 	}
 
 	uefi_set_attrs = shim_mem_attrs_to_uefi_mem_attrs (set_attrs);
@@ -140,9 +345,9 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
 	}
 	ret = efi_status;
 
-	efi_status = get_mem_attrs (addr, size, &after);
+	efi_status = efi_get_mem_attrs(addr, size, &after);
 	if (EFI_ERROR(efi_status))
-		dprint(L"get_mem_attrs(0x%llx, %llu, 0x%llx) -> 0x%lx\n",
+		dprint(L"efi_get_mem_attrs(0x%llx, %llu, 0x%llx) -> 0x%lx\n",
 		       (unsigned long long)addr, (unsigned long long)size,
 		       &after, efi_status);
 
@@ -164,6 +369,37 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
 	return ret;
 }
 
+EFI_STATUS
+update_mem_attrs(uintptr_t addr, uint64_t size,
+		 uint64_t set_attrs, uint64_t clear_attrs)
+{
+	EFI_STATUS efi_status;
+
+	efi_status = efi_update_mem_attrs(addr, size, set_attrs, clear_attrs);
+	if (!EFI_ERROR(efi_status))
+		return efi_status;
+
+	if (efi_status == EFI_UNSUPPORTED)
+		efi_status = dxe_update_mem_attrs(addr, size, set_attrs, clear_attrs);
+
+	return efi_status;
+}
+
+EFI_STATUS
+get_mem_attrs(uintptr_t addr, size_t size, uint64_t *attrs)
+{
+	EFI_STATUS efi_status;
+
+	efi_status = efi_get_mem_attrs(addr, size, attrs);
+	if (!EFI_ERROR(efi_status))
+		return efi_status;
+
+	if (efi_status == EFI_UNSUPPORTED)
+		efi_status = dxe_get_mem_attrs(addr, size, attrs);
+
+	return efi_status;
+}
+
 void
 get_hsi_mem_info(void)
 {
@@ -171,22 +407,10 @@ get_hsi_mem_info(void)
 	uintptr_t addr;
 	uint64_t attrs = 0;
 	uint32_t *tmp_alloc;
+	EFI_MEMORY_ATTRIBUTE_PROTOCOL *efiproto = NULL;
 
-	addr = ((uintptr_t)&get_hsi_mem_info) & ~EFI_PAGE_MASK;
-
-	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
-	if (EFI_ERROR(efi_status)) {
-error:
-		/*
-		 * In this case we can't actually tell anything, so assume
-		 * and report the worst case scenario.
-		 */
-		hsi_status = SHIM_HSI_STATUS_HEAPX |
-			     SHIM_HSI_STATUS_STACKX |
-			     SHIM_HSI_STATUS_ROW;
-		dprint(L"Setting HSI to 0x%lx due to error: %r\n", hsi_status, efi_status);
-		return;
-	} else {
+	get_efi_mem_attr_protocol(&efiproto);
+	if (efiproto) {
 		hsi_status = SHIM_HSI_STATUS_HASMAP;
 		dprint(L"Setting HSI to 0x%lx\n", hsi_status);
 	}
@@ -196,7 +420,13 @@ error:
 		return;
 	}
 
-	hsi_status = SHIM_HSI_STATUS_HASMAP;
+	addr = ((uintptr_t)&get_hsi_mem_info) & ~EFI_PAGE_MASK;
+	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"get_mem_attrs(0x%016llx, 0x%x, &attrs) failed.\n", addr, EFI_PAGE_SIZE);
+		goto error;
+	}
+
 	if (attrs & MEM_ATTR_W) {
 		dprint(L"get_hsi_mem_info() is on a writable page: 0x%x->0x%x\n",
 		       hsi_status, hsi_status | SHIM_HSI_STATUS_ROW);
@@ -234,6 +464,18 @@ error:
 		       hsi_status, hsi_status | SHIM_HSI_STATUS_HEAPX);
 		hsi_status |= SHIM_HSI_STATUS_HEAPX;
 	}
+
+	return;
+
+error:
+	/*
+	 * In this case we can't actually tell anything, so assume
+	 * and report the worst case scenario.
+	 */
+	hsi_status = SHIM_HSI_STATUS_HEAPX |
+		     SHIM_HSI_STATUS_STACKX |
+		     SHIM_HSI_STATUS_ROW;
+	dprint(L"Setting HSI to 0x%lx due to error: %r\n", hsi_status, efi_status);
 }
 
 // vim:fenc=utf-8:tw=75:noet
-- 
cgit v1.2.3


From 9269e9b0aa15ae9832f7eba6c5eeef0c5e1f4edb Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 20 Feb 2025 14:44:10 -0500
Subject: Add DXE Services information to HSI

This adds three more entries to our HSI data:

has-dxe-services-table: technically only tells us if UEFI's
			LocateProtocol will give us a DXE services
			table, but practically also tells us if the
			machine is implementing DXE in any way.
has-get-memory-space-descriptor: tells us if DXE->GetMemorySpaceDescriptor
                                 is populated
has-set-memory-space-descriptor: tells us if DXE->SetMemorySpaceDescriptor
                                 is populated

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 MokVars.txt                   |  6 ++++++
 include/mok.h                 |  6 ++++++
 include/test-data-efivars-1.h |  6 +++++-
 memattrs.c                    | 16 ++++++++++++++--
 mok.c                         | 11 +++++++++++
 5 files changed, 42 insertions(+), 3 deletions(-)

(limited to 'memattrs.c')

diff --git a/MokVars.txt b/MokVars.txt
index e6e68ce4..0ab81ff6 100644
--- a/MokVars.txt
+++ b/MokVars.txt
@@ -103,3 +103,9 @@ HSIStatus: Status of various security features:
                             1: read-only sections are writable
   has-memory-attribute-protocol: 0: platform does not provide the EFI Memory Attribute Protocol
                                  1: platform does provide the EFI Memory Attribute Protocol
+  has-dxe-services-table: 0: platform does not provide the DXE Services Table
+                          1: platform does provide the DXE Services Table
+  has-get-memory-space-descriptor: 0: platform's DST does not populate GetMemorySpaceDescriptor
+                                   1: platform's DST does populate GetMemorySpaceDescriptor
+  has-set-memory-space-descriptor: 0: platform's DST does not populate SetMemorySpaceDescriptor
+                                   1: platform's DST does populate SetMemorySpaceDescriptor
diff --git a/include/mok.h b/include/mok.h
index 1b44217c..cea4c997 100644
--- a/include/mok.h
+++ b/include/mok.h
@@ -141,6 +141,12 @@ extern UINTN hsi_status;
 #define SHIM_HSI_STATUS_ROW		0x00000004ULL
 /* platform provides the EFI Memory Attribute Protocol */
 #define SHIM_HSI_STATUS_HASMAP		0x00000008ULL
+/* platform provides DXE Services Table */
+#define SHIM_HSI_STATUS_HASDST		0x00000010ULL
+/* platform has DST->GetMemorySpaceDescriptor */
+#define SHIM_HSI_STATUS_HASDSTGMSD	0x00000020ULL
+/* platform has DST->SetMemorySpaceAttributes */
+#define SHIM_HSI_STATUS_HASDSTSMSA	0x00000040ULL
 
 #endif /* !SHIM_MOK_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/include/test-data-efivars-1.h b/include/test-data-efivars-1.h
index d97a4d6d..7a34ea70 100644
--- a/include/test-data-efivars-1.h
+++ b/include/test-data-efivars-1.h
@@ -110,7 +110,11 @@ static const unsigned char test_data_efivars_1_HSIStatus[] =
 	"heap-is-executable: 0\n"
 	"stack-is-executable: 0\n"
 	"ro-sections-are-writable: 0\n"
-	"has-memory-attribute-protocol: 0\n";
+	"has-memory-attribute-protocol: 0\n"
+	"has-dxe-services-table: 0\n"
+	"has-get-memory-space-descriptor: 0\n"
+	"has-set-memory-space-attributes: 0\n"
+	;
 
 #endif /* !TEST_DATA_EFIVARS_1_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/memattrs.c b/memattrs.c
index a2c1777c..f502805f 100644
--- a/memattrs.c
+++ b/memattrs.c
@@ -50,7 +50,7 @@ get_dxe_services_table(EFI_DXE_SERVICES_TABLE **dstp)
 	static EFI_DXE_SERVICES_TABLE *dst = NULL;
 
 	if (dst == NULL) {
-	dprint(L"Looking for configuration table " LGUID_FMT L"\n", GUID_ARGS(gEfiDxeServicesTableGuid));
+		dprint(L"Looking for configuration table " LGUID_FMT L"\n", GUID_ARGS(gEfiDxeServicesTableGuid));
 
 		for (UINTN i = 0; i < ST->NumberOfTableEntries; i++) {
 			EFI_CONFIGURATION_TABLE *ct = &ST->ConfigurationTable[i];
@@ -408,6 +408,7 @@ get_hsi_mem_info(void)
 	uint64_t attrs = 0;
 	uint32_t *tmp_alloc;
 	EFI_MEMORY_ATTRIBUTE_PROTOCOL *efiproto = NULL;
+	EFI_DXE_SERVICES_TABLE *dst = NULL;
 
 	get_efi_mem_attr_protocol(&efiproto);
 	if (efiproto) {
@@ -415,7 +416,18 @@ get_hsi_mem_info(void)
 		dprint(L"Setting HSI to 0x%lx\n", hsi_status);
 	}
 
-	if (!(hsi_status & SHIM_HSI_STATUS_HASMAP)) {
+	get_dxe_services_table(&dst);
+	if (dst) {
+		hsi_status |= SHIM_HSI_STATUS_HASDST;
+		if (dst->GetMemorySpaceDescriptor)
+			hsi_status |= SHIM_HSI_STATUS_HASDSTGMSD;
+		if (dst->SetMemorySpaceAttributes)
+			hsi_status |= SHIM_HSI_STATUS_HASDSTSMSA;
+	}
+
+	if (!(hsi_status & SHIM_HSI_STATUS_HASMAP) &&
+	    !(hsi_status & SHIM_HSI_STATUS_HASDSTGMSD &&
+	      hsi_status & SHIM_HSI_STATUS_HASDSTSMSA)) {
 		dprint(L"No memory protocol, not testing further\n");
 		return;
 	}
diff --git a/mok.c b/mok.c
index 97d4a0eb..cb70e7e2 100644
--- a/mok.c
+++ b/mok.c
@@ -42,6 +42,9 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	const char stackx[] = "\nstack-is-executable: ";
 	const char row[] = "\nro-sections-are-writable: ";
 	const char hasmap[] = "\nhas-memory-attribute-protocol: ";
+	const char hasdxeservices[] = "\nhas-dxe-services-table: ";
+	const char hasdsgmsd[] = "\nhas-get-memory-space-descriptor: ";
+	const char hasdssmsa[] = "\nhas-set-memory-space-attributes: ";
 	const char finale[] = "\n";
 	char *pos;
 
@@ -51,6 +54,8 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	 */
 	UINTN ret = sizeof(heapx) + sizeof(stackx) +
 		    sizeof(row) + sizeof(hasmap) +
+		    sizeof(hasdxeservices) + sizeof(hasdsgmsd) +
+		    sizeof(hasdssmsa) +
 		    sizeof(finale);
 
 	if (buf == 0 || sz < ret) {
@@ -67,6 +72,12 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_ROW) ? "1" : "0");
 	pos = stpcpy(pos, hasmap);
 	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASMAP) ? "1" : "0");
+	pos = stpcpy(pos, hasdxeservices);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASDST) ? "1" : "0");
+	pos = stpcpy(pos, hasdsgmsd);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASDSTGMSD) ? "1" : "0");
+	pos = stpcpy(pos, hasdssmsa);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASDSTSMSA) ? "1" : "0");
 	stpcpy(pos, finale);
 
 	return ret;
-- 
cgit v1.2.3


From 1baf1efb37e2728104765477b12b70aeef3090af Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Tue, 25 Feb 2025 10:41:41 -0500
Subject: HSI: Add decode_hsi_bits() for easier reading of the debug log

This changes all the HSI bitfield operations to print a string showing
the change instead of just hex values.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 include/memattrs.h |  1 +
 memattrs.c         | 78 ++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 68 insertions(+), 11 deletions(-)

(limited to 'memattrs.c')

diff --git a/include/memattrs.h b/include/memattrs.h
index 5c40b4cc..193da988 100644
--- a/include/memattrs.h
+++ b/include/memattrs.h
@@ -12,6 +12,7 @@ extern EFI_STATUS update_mem_attrs(uintptr_t addr, uint64_t size,
 				   uint64_t set_attrs, uint64_t clear_attrs);
 
 extern void get_hsi_mem_info(void);
+extern char *decode_hsi_bits(UINTN hsi);
 
 #endif /* !SHIM_MEMATTRS_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/memattrs.c b/memattrs.c
index f502805f..bc571641 100644
--- a/memattrs.c
+++ b/memattrs.c
@@ -400,6 +400,46 @@ get_mem_attrs(uintptr_t addr, size_t size, uint64_t *attrs)
 	return efi_status;
 }
 
+char *
+decode_hsi_bits(UINTN hsi)
+{
+	static const struct {
+		UINTN bit;
+		char name[16];
+	} bits[] = {
+		{.bit = SHIM_HSI_STATUS_HEAPX, .name = "HEAPX"},
+		{.bit = SHIM_HSI_STATUS_STACKX, .name = "STACKX"},
+		{.bit = SHIM_HSI_STATUS_ROW, .name = "ROW"},
+		{.bit = SHIM_HSI_STATUS_HASMAP, .name = "HASMAP"},
+		{.bit = SHIM_HSI_STATUS_HASDST, .name = "HASDST"},
+		{.bit = SHIM_HSI_STATUS_HASDSTGMSD, .name = "HASDSTGMSD"},
+		{.bit = SHIM_HSI_STATUS_HASDSTSMSA, .name = "HASDSTSMSA"},
+		{.bit = 0, .name = ""},
+	};
+	static int x = 0;
+	static char retbufs[2][sizeof(bits)];
+	char *retbuf = &retbufs[x % 2][0];
+	char *prev = &retbuf[0];
+	char *pos = &retbuf[0];
+
+	x = ( x + 1 ) % 2;
+
+	ZeroMem(retbuf, sizeof(bits));
+
+	if (hsi == 0) {
+		prev = stpcpy(retbuf, "0");
+	} else {
+		for (UINTN i = 0; bits[i].bit != 0; i++) {
+			if (hsi & bits[i].bit) {
+				prev = stpcpy(pos, bits[i].name);
+				pos = stpcpy(prev, "|");
+			}
+		}
+	}
+	prev[0] = '\0';
+	return retbuf;
+}
+
 void
 get_hsi_mem_info(void)
 {
@@ -412,17 +452,30 @@ get_hsi_mem_info(void)
 
 	get_efi_mem_attr_protocol(&efiproto);
 	if (efiproto) {
-		hsi_status = SHIM_HSI_STATUS_HASMAP;
-		dprint(L"Setting HSI to 0x%lx\n", hsi_status);
+		dprint(L"Setting HSI from %a to %a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_HASMAP));
+		hsi_status |= SHIM_HSI_STATUS_HASMAP;
 	}
 
 	get_dxe_services_table(&dst);
 	if (dst) {
+		dprint(L"Setting HSI from %a to %a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_HASDST));
 		hsi_status |= SHIM_HSI_STATUS_HASDST;
-		if (dst->GetMemorySpaceDescriptor)
+		if (dst->GetMemorySpaceDescriptor) {
+			dprint(L"Setting HSI from %a to %a\n",
+			       decode_hsi_bits(hsi_status),
+			       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_HASDSTGMSD));
 			hsi_status |= SHIM_HSI_STATUS_HASDSTGMSD;
-		if (dst->SetMemorySpaceAttributes)
+		}
+		if (dst->SetMemorySpaceAttributes) {
+			dprint(L"Setting HSI from %a to %a\n",
+			       decode_hsi_bits(hsi_status),
+			       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_HASDSTSMSA));
 			hsi_status |= SHIM_HSI_STATUS_HASDSTSMSA;
+		}
 	}
 
 	if (!(hsi_status & SHIM_HSI_STATUS_HASMAP) &&
@@ -440,8 +493,9 @@ get_hsi_mem_info(void)
 	}
 
 	if (attrs & MEM_ATTR_W) {
-		dprint(L"get_hsi_mem_info() is on a writable page: 0x%x->0x%x\n",
-		       hsi_status, hsi_status | SHIM_HSI_STATUS_ROW);
+		dprint(L"get_hsi_mem_info() is on a writable page: %a->%a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_ROW));
 		hsi_status |= SHIM_HSI_STATUS_ROW;
 	}
 
@@ -453,8 +507,9 @@ get_hsi_mem_info(void)
 	}
 
 	if (attrs & MEM_ATTR_X) {
-		dprint(L"Stack variable is on an executable page: 0x%x->0x%x\n",
-		       hsi_status, hsi_status | SHIM_HSI_STATUS_STACKX);
+		dprint(L"Stack variable is on an executable page: %a->%a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_STACKX));
 		hsi_status |= SHIM_HSI_STATUS_STACKX;
 	}
 
@@ -472,8 +527,9 @@ get_hsi_mem_info(void)
 		goto error;
 	}
 	if (attrs & MEM_ATTR_X) {
-		dprint(L"Heap variable is on an executable page: 0x%x->0x%x\n",
-		       hsi_status, hsi_status | SHIM_HSI_STATUS_HEAPX);
+		dprint(L"Heap variable is on an executable page: %a->%a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_HEAPX));
 		hsi_status |= SHIM_HSI_STATUS_HEAPX;
 	}
 
@@ -487,7 +543,7 @@ error:
 	hsi_status = SHIM_HSI_STATUS_HEAPX |
 		     SHIM_HSI_STATUS_STACKX |
 		     SHIM_HSI_STATUS_ROW;
-	dprint(L"Setting HSI to 0x%lx due to error: %r\n", hsi_status, efi_status);
+	dprint(L"Setting HSI to 0x%lx due to error: %r\n", decode_hsi_bits(hsi_status), efi_status);
 }
 
 // vim:fenc=utf-8:tw=75:noet
-- 
cgit v1.2.3


From 89e615081af5fbafefeae5b09def3a003e467838 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 20 Feb 2025 19:20:47 -0500
Subject: Add shim's current NX_COMPAT status to HSIStatus

hughsie asked me to also make it observable at runtime whether the shim
binary that was used to boot was set as NX_COMPAT or not.

This adds that into the HSIStatus data as "shim-has-nx-compat-set".

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 MokVars.txt                   |  2 ++
 include/mok.h                 |  2 ++
 include/pe.h                  |  3 +++
 include/test-data-efivars-1.h |  1 +
 memattrs.c                    |  1 +
 mok.c                         |  5 ++++-
 pe-relocate.c                 | 37 +++++++++++++++++++++++++++++++++----
 shim.c                        |  2 ++
 8 files changed, 48 insertions(+), 5 deletions(-)

(limited to 'memattrs.c')

diff --git a/MokVars.txt b/MokVars.txt
index 0ab81ff6..6ad8ce70 100644
--- a/MokVars.txt
+++ b/MokVars.txt
@@ -109,3 +109,5 @@ HSIStatus: Status of various security features:
                                    1: platform's DST does populate GetMemorySpaceDescriptor
   has-set-memory-space-descriptor: 0: platform's DST does not populate SetMemorySpaceDescriptor
                                    1: platform's DST does populate SetMemorySpaceDescriptor
+  shim-has-nx-compat-set: 0: the running shim binary does not have NX_COMPAT bit set
+                          1: the running shim binary does have the NX_COMPAT bit set
diff --git a/include/mok.h b/include/mok.h
index cea4c997..89edf9de 100644
--- a/include/mok.h
+++ b/include/mok.h
@@ -147,6 +147,8 @@ extern UINTN hsi_status;
 #define SHIM_HSI_STATUS_HASDSTGMSD	0x00000020ULL
 /* platform has DST->SetMemorySpaceAttributes */
 #define SHIM_HSI_STATUS_HASDSTSMSA	0x00000040ULL
+/* This shim has the NX_COMPAT bit set */
+#define SHIM_HSI_STATUS_NX		0x00000100ULL
 
 #endif /* !SHIM_MOK_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/include/pe.h b/include/pe.h
index a1eb8853..ea40184b 100644
--- a/include/pe.h
+++ b/include/pe.h
@@ -53,5 +53,8 @@ relocate_coff (PE_COFF_LOADER_IMAGE_CONTEXT *context,
 	       EFI_IMAGE_SECTION_HEADER *Section,
 	       void *orig, void *data);
 
+void
+get_shim_nx_capability(EFI_HANDLE image_handle);
+
 #endif /* !PE_H_ */
 // vim:fenc=utf-8:tw=75:noet
diff --git a/include/test-data-efivars-1.h b/include/test-data-efivars-1.h
index 7a34ea70..259558e0 100644
--- a/include/test-data-efivars-1.h
+++ b/include/test-data-efivars-1.h
@@ -114,6 +114,7 @@ static const unsigned char test_data_efivars_1_HSIStatus[] =
 	"has-dxe-services-table: 0\n"
 	"has-get-memory-space-descriptor: 0\n"
 	"has-set-memory-space-attributes: 0\n"
+	"shim-has-nx-compat-set: 0\n"
 	;
 
 #endif /* !TEST_DATA_EFIVARS_1_H_ */
diff --git a/memattrs.c b/memattrs.c
index bc571641..ed8a3ae2 100644
--- a/memattrs.c
+++ b/memattrs.c
@@ -414,6 +414,7 @@ decode_hsi_bits(UINTN hsi)
 		{.bit = SHIM_HSI_STATUS_HASDST, .name = "HASDST"},
 		{.bit = SHIM_HSI_STATUS_HASDSTGMSD, .name = "HASDSTGMSD"},
 		{.bit = SHIM_HSI_STATUS_HASDSTSMSA, .name = "HASDSTSMSA"},
+		{.bit = SHIM_HSI_STATUS_NX, .name = "NX"},
 		{.bit = 0, .name = ""},
 	};
 	static int x = 0;
diff --git a/mok.c b/mok.c
index cb70e7e2..fb4c1489 100644
--- a/mok.c
+++ b/mok.c
@@ -45,6 +45,7 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	const char hasdxeservices[] = "\nhas-dxe-services-table: ";
 	const char hasdsgmsd[] = "\nhas-get-memory-space-descriptor: ";
 	const char hasdssmsa[] = "\nhas-set-memory-space-attributes: ";
+	const char shimhasnx[] = "\nshim-has-nx-compat-set: ";
 	const char finale[] = "\n";
 	char *pos;
 
@@ -55,7 +56,7 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	UINTN ret = sizeof(heapx) + sizeof(stackx) +
 		    sizeof(row) + sizeof(hasmap) +
 		    sizeof(hasdxeservices) + sizeof(hasdsgmsd) +
-		    sizeof(hasdssmsa) +
+		    sizeof(hasdssmsa) + sizeof(shimhasnx) +
 		    sizeof(finale);
 
 	if (buf == 0 || sz < ret) {
@@ -78,6 +79,8 @@ format_hsi_status(UINT8 *buf, size_t sz,
 	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASDSTGMSD) ? "1" : "0");
 	pos = stpcpy(pos, hasdssmsa);
 	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_HASDSTSMSA) ? "1" : "0");
+	pos = stpcpy(pos, shimhasnx);
+	pos = stpcpy(pos, (hsi_status & SHIM_HSI_STATUS_NX) ? "1" : "0");
 	stpcpy(pos, finale);
 
 	return ret;
diff --git a/pe-relocate.c b/pe-relocate.c
index b436d3ec..4991dbe1 100644
--- a/pe-relocate.c
+++ b/pe-relocate.c
@@ -375,7 +375,6 @@ read_header(void *data, unsigned int datasize,
 	EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr = data;
 	unsigned long HeaderWithoutDataDir, SectionHeaderOffset, OptHeaderSize;
 	unsigned long FileAlignment = 0;
-	UINT16 DllFlags;
 	size_t dos_sz = 0;
 	size_t tmpsz0, tmpsz1;
 
@@ -504,17 +503,17 @@ read_header(void *data, unsigned int datasize,
 		context->EntryPoint = PEHdr->Pe32Plus.OptionalHeader.AddressOfEntryPoint;
 		context->RelocDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
 		context->SecDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
-		DllFlags = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics;
+		context->DllCharacteristics = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics;
 	} else {
 		context->ImageAddress = PEHdr->Pe32.OptionalHeader.ImageBase;
 		context->EntryPoint = PEHdr->Pe32.OptionalHeader.AddressOfEntryPoint;
 		context->RelocDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
 		context->SecDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
-		DllFlags = PEHdr->Pe32.OptionalHeader.DllCharacteristics;
+		context->DllCharacteristics = PEHdr->Pe32.OptionalHeader.DllCharacteristics;
 	}
 
 	if ((mok_policy & MOK_POLICY_REQUIRE_NX) &&
-	    !(DllFlags & EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT)) {
+	    !(context->DllCharacteristics & EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT)) {
 		perror(L"Policy requires NX, but image does not support NX\n");
 		return EFI_UNSUPPORTED;
         }
@@ -555,4 +554,34 @@ read_header(void *data, unsigned int datasize,
 	return EFI_SUCCESS;
 }
 
+void
+get_shim_nx_capability(EFI_HANDLE image_handle)
+{
+	EFI_LOADED_IMAGE_PROTOCOL*li = NULL;
+	EFI_STATUS efi_status;
+	PE_COFF_LOADER_IMAGE_CONTEXT context;
+
+	efi_status = BS->HandleProtocol(image_handle, &gEfiLoadedImageProtocolGuid, (void **)&li);
+	if (EFI_ERROR(efi_status) || !li) {
+		dprint(L"Could not get loaded image protocol: %r\n", efi_status);
+		return;
+	}
+
+	ZeroMem(&context, sizeof(context));
+	efi_status = read_header(li->ImageBase, li->ImageSize, &context, false);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"Couldn't parse image header: %r\n", efi_status);
+		return;
+	}
+
+	dprint(L"DllCharacteristics:0x%lx\n", context.DllCharacteristics);
+	if (context.DllCharacteristics & EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT) {
+		dprint(L"Setting HSI from %a to %a\n",
+		       decode_hsi_bits(hsi_status),
+		       decode_hsi_bits(hsi_status | SHIM_HSI_STATUS_NX));
+		hsi_status |= SHIM_HSI_STATUS_NX;
+	}
+}
+
+
 // vim:fenc=utf-8:tw=75:noet
diff --git a/shim.c b/shim.c
index 9efbde11..f85ce00e 100644
--- a/shim.c
+++ b/shim.c
@@ -1993,6 +1993,8 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab)
 	 */
 	debug_hook();
 
+	get_shim_nx_capability(image_handle);
+
 	efi_status = set_sbat_uefi_variable_internal();
 	if (EFI_ERROR(efi_status) && secure_mode()) {
 		perror(L"%s variable initialization failed\n", SBAT_VAR_NAME);
-- 
cgit v1.2.3