[dpdk-dev] [kmods PATCH v2 2/4] windows/virt2phys: do not expose pageable physical addresses

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

virt2phys relied on the user to ensure that memory for which physical
address (PA) is obtained is non-pageable and remains such
for the lifetime of the process. While DPDK does lock pages in memory,
virt2phys can be accessed by any process with sufficient privileges.
A malicious process could get PA and make memory pageable. When it is
reused for another process, attacker can get access to private data
or crash the system if another process is kernel.

Solution is to lock all memory for which PA is requested.
Locked blocks are tracked to unlock them when the process exits.
If driver is unloaded while some memory is still locked, it leaves pages
locked, effectively leaking RAM, trading stability for security.

Factor out a module that locks memory for which physical addresses
are obtained and keeps a list of locked memory blocks for each process.
It exposes a thread-safe interface for use in driver callbacks.
The driver reacts to a process exit by unlocking its tracked blocks.

Also clean up the driver code. Remove debugging output to replace it
with structured tracing in the next patch.

Reported-by: Dmitry Malloy <dmitrym@microsoft.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
There's a couple of line length violations, but 80 characters
is not a hard limit now and also those lines are ideomatic,
splitting won't help readability.

 windows/virt2phys/virt2phys.c               |  92 +++--
 windows/virt2phys/virt2phys.vcxproj         |   2 +
 windows/virt2phys/virt2phys.vcxproj.filters |   6 +
 windows/virt2phys/virt2phys_logic.c         | 368 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  32 ++
 5 files changed, 470 insertions(+), 30 deletions(-)
 create mode 100644 windows/virt2phys/virt2phys_logic.c
 create mode 100644 windows/virt2phys/virt2phys_logic.h

diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index e157e9c..bf0c300 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -1,21 +1,25 @@
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2020 Dmitry Kozlyuk
+ * Copyright 2020-2021 Dmitry Kozlyuk
  */
 
 #include <ntddk.h>
 #include <wdf.h>
-#include <wdmsec.h>
 #include <initguid.h>
 
 #include "virt2phys.h"
+#include "virt2phys_logic.h"
 
 DRIVER_INITIALIZE DriverEntry;
+EVT_WDF_DRIVER_UNLOAD virt2phys_driver_unload;
 EVT_WDF_DRIVER_DEVICE_ADD virt2phys_driver_EvtDeviceAdd;
 EVT_WDF_IO_IN_CALLER_CONTEXT virt2phys_device_EvtIoInCallerContext;
 
+static VOID virt2phys_on_process_event(
+	HANDLE parent_id, HANDLE process_id, BOOLEAN create);
+
+_Use_decl_annotations_
 NTSTATUS
-DriverEntry(
-	IN PDRIVER_OBJECT driver_object, IN PUNICODE_STRING registry_path)
+DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 {
 	WDF_DRIVER_CONFIG config;
 	WDF_OBJECT_ATTRIBUTES attributes;
@@ -23,22 +27,51 @@ DriverEntry(
 
 	PAGED_CODE();
 
-	WDF_DRIVER_CONFIG_INIT(&config, virt2phys_driver_EvtDeviceAdd);
 	WDF_OBJECT_ATTRIBUTES_INIT(&attributes);
+	WDF_DRIVER_CONFIG_INIT(&config, virt2phys_driver_EvtDeviceAdd);
+	config.EvtDriverUnload = virt2phys_driver_unload;
 	status = WdfDriverCreate(
-			driver_object, registry_path,
-			&attributes, &config, WDF_NO_HANDLE);
-	if (!NT_SUCCESS(status)) {
-		KdPrint(("WdfDriverCreate() failed, status=%08x\n", status));
-	}
+		driver_object, registry_path,
+		&attributes, &config, WDF_NO_HANDLE);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_init();
+	if (!NT_SUCCESS(status))
+		return status;
+
+	/*
+	 * The goal is to ensure that no process obtains a physical address
+	 * of pageable memory. To do this the driver locks every memory region
+	 * for which physical address is requested. This memory must remain
+	 * locked until process has no access to it anymore. A process can use
+	 * the memory after it closes all handles to the interface device,
+	 * so the driver cannot unlock memory at device cleanup callback.
+	 * It has to track process termination instead, after which point
+	 * a process cannot attempt any memory access.
+	 */
+	status = PsSetCreateProcessNotifyRoutine(
+		virt2phys_on_process_event, FALSE);
+	if (!NT_SUCCESS(status))
+		return status;
 
 	return status;
 }
 
+_Use_decl_annotations_
+VOID
+virt2phys_driver_unload(WDFDRIVER driver)
+{
+	UNREFERENCED_PARAMETER(driver);
+
+	PsSetCreateProcessNotifyRoutine(virt2phys_on_process_event, TRUE);
+
+	virt2phys_cleanup();
+}
+
 _Use_decl_annotations_
 NTSTATUS
-virt2phys_driver_EvtDeviceAdd(
-	WDFDRIVER driver, PWDFDEVICE_INIT init)
+virt2phys_driver_EvtDeviceAdd(WDFDRIVER driver, PWDFDEVICE_INIT init)
 {
 	WDF_OBJECT_ATTRIBUTES attributes;
 	WDFDEVICE device;
@@ -46,8 +79,6 @@ virt2phys_driver_EvtDeviceAdd(
 
 	UNREFERENCED_PARAMETER(driver);
 
-	PAGED_CODE();
-
 	WdfDeviceInitSetIoType(
 		init, WdfDeviceIoNeither);
 	WdfDeviceInitSetIoInCallerContextCallback(
@@ -57,15 +88,12 @@ virt2phys_driver_EvtDeviceAdd(
 
 	status = WdfDeviceCreate(&init, &attributes, &device);
 	if (!NT_SUCCESS(status)) {
-		KdPrint(("WdfDeviceCreate() failed, status=%08x\n", status));
 		return status;
 	}
 
 	status = WdfDeviceCreateDeviceInterface(
-			device, &GUID_DEVINTERFACE_VIRT2PHYS, NULL);
+		device, &GUID_DEVINTERFACE_VIRT2PHYS, NULL);
 	if (!NT_SUCCESS(status)) {
-		KdPrint(("WdfDeviceCreateDeviceInterface() failed, "
-			"status=%08x\n", status));
 		return status;
 	}
 
@@ -74,8 +102,7 @@ virt2phys_driver_EvtDeviceAdd(
 
 _Use_decl_annotations_
 VOID
-virt2phys_device_EvtIoInCallerContext(
-	IN WDFDEVICE device, IN WDFREQUEST request)
+virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 {
 	WDF_REQUEST_PARAMETERS params;
 	ULONG code;
@@ -85,21 +112,18 @@ virt2phys_device_EvtIoInCallerContext(
 	NTSTATUS status;
 
 	UNREFERENCED_PARAMETER(device);
-
 	PAGED_CODE();
 
 	WDF_REQUEST_PARAMETERS_INIT(&params);
 	WdfRequestGetParameters(request, &params);
 
 	if (params.Type != WdfRequestTypeDeviceControl) {
-		KdPrint(("bogus request type=%u\n", params.Type));
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
 
 	code = params.Parameters.DeviceIoControl.IoControlCode;
 	if (code != IOCTL_VIRT2PHYS_TRANSLATE) {
-		KdPrint(("bogus IO control code=%lu\n", code));
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
@@ -107,8 +131,6 @@ virt2phys_device_EvtIoInCallerContext(
 	status = WdfRequestRetrieveInputBuffer(
 			request, sizeof(*virt), (PVOID *)&virt, &size);
 	if (!NT_SUCCESS(status)) {
-		KdPrint(("WdfRequestRetrieveInputBuffer() failed, "
-			"status=%08x\n", status));
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -116,14 +138,24 @@ virt2phys_device_EvtIoInCallerContext(
 	status = WdfRequestRetrieveOutputBuffer(
 		request, sizeof(*phys), (PVOID *)&phys, &size);
 	if (!NT_SUCCESS(status)) {
-		KdPrint(("WdfRequestRetrieveOutputBuffer() failed, "
-			"status=%08x\n", status));
 		WdfRequestComplete(request, status);
 		return;
 	}
 
-	*phys = MmGetPhysicalAddress(*virt);
+	status = virt2phys_translate(*virt, phys);
+	if (NT_SUCCESS(status))
+		WdfRequestSetInformation(request, sizeof(*phys));
+	WdfRequestComplete(request, status);
+}
+
+static VOID
+virt2phys_on_process_event(
+	HANDLE parent_id, HANDLE process_id, BOOLEAN create)
+{
+	UNREFERENCED_PARAMETER(parent_id);
+
+	if (create)
+		return;
 
-	WdfRequestCompleteWithInformation(
-		request, STATUS_SUCCESS, sizeof(*phys));
+	virt2phys_process_cleanup(process_id);
 }
diff --git a/windows/virt2phys/virt2phys.vcxproj b/windows/virt2phys/virt2phys.vcxproj
index c86cc9b..b462493 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -36,9 +36,11 @@
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virt2phys.c" />
+    <ClCompile Include="virt2phys_logic.c" />
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="virt2phys.h" />
+    <ClInclude Include="virt2phys_logic.h" />
   </ItemGroup>
   <ItemGroup>
     <Inf Include="virt2phys.inf" />
diff --git a/windows/virt2phys/virt2phys.vcxproj.filters b/windows/virt2phys/virt2phys.vcxproj.filters
index 9e7e732..acd159f 100644
--- a/windows/virt2phys/virt2phys.vcxproj.filters
+++ b/windows/virt2phys/virt2phys.vcxproj.filters
@@ -27,10 +27,16 @@
     <ClInclude Include="virt2phys.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="virt2phys_logic.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virt2phys.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="virt2phys_logic.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
 </Project>
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
new file mode 100644
index 0000000..a27802c
--- /dev/null
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -0,0 +1,368 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Dmitry Kozlyuk
+ */
+
+#include <ntifs.h>
+#include <ntddk.h>
+
+#include "virt2phys_logic.h"
+
+struct virt2phys_process {
+	HANDLE id;
+	LIST_ENTRY next;
+	SINGLE_LIST_ENTRY blocks;
+};
+
+struct virt2phys_block {
+	PMDL mdl;
+	SINGLE_LIST_ENTRY next;
+};
+
+static LIST_ENTRY g_processes;
+static PKSPIN_LOCK g_lock;
+
+struct virt2phys_block *
+virt2phys_block_create(PMDL mdl)
+{
+	struct virt2phys_block *block;
+
+	block = ExAllocatePoolZero(NonPagedPool, sizeof(*block), 'bp2v');
+	if (block != NULL)
+		block->mdl = mdl;
+	return block;
+}
+
+static void
+virt2phys_block_free(struct virt2phys_block *block, BOOLEAN unmap)
+{
+	if (unmap)
+		MmUnlockPages(block->mdl);
+
+	IoFreeMdl(block->mdl);
+	ExFreePool(block);
+}
+
+static PHYSICAL_ADDRESS
+virt2phys_block_translate(struct virt2phys_block *block, PVOID virt)
+{
+	PPFN_NUMBER pfn;
+	PVOID base;
+	PHYSICAL_ADDRESS phys;
+
+	pfn = MmGetMdlPfnArray(block->mdl);
+	base = MmGetMdlVirtualAddress(block->mdl);
+	phys.QuadPart = pfn[0] * PAGE_SIZE +
+		((uintptr_t)virt - (uintptr_t)base);
+	return phys;
+}
+
+static struct virt2phys_process *
+virt2phys_process_create(HANDLE process_id)
+{
+	struct virt2phys_process *process;
+
+	process = ExAllocatePoolZero(NonPagedPool, sizeof(*process), 'pp2v');
+	if (process != NULL)
+		process->id = process_id;
+	return process;
+}
+
+static void
+virt2phys_process_free(struct virt2phys_process *process, BOOLEAN unmap)
+{
+	PSINGLE_LIST_ENTRY node;
+	struct virt2phys_block *block;
+
+	node = process->blocks.Next;
+	while (node != NULL) {
+		block = CONTAINING_RECORD(node, struct virt2phys_block, next);
+		node = node->Next;
+		virt2phys_block_free(block, unmap);
+	}
+
+	ExFreePool(process);
+}
+
+static struct virt2phys_process *
+virt2phys_process_find(HANDLE process_id)
+{
+	PLIST_ENTRY node;
+	struct virt2phys_process *cur;
+
+	for (node = g_processes.Flink; node != &g_processes; node = node->Flink) {
+		cur = CONTAINING_RECORD(node, struct virt2phys_process, next);
+		if (cur->id == process_id)
+			return cur;
+	}
+	return NULL;
+}
+
+static struct virt2phys_block *
+virt2phys_process_find_block(struct virt2phys_process *process, PVOID virt)
+{
+	PSINGLE_LIST_ENTRY node;
+	struct virt2phys_block *cur;
+
+	for (node = process->blocks.Next; node != NULL; node = node->Next) {
+		cur = CONTAINING_RECORD(node, struct virt2phys_block, next);
+		if (cur->mdl->StartVa == virt)
+			return cur;
+	}
+	return NULL;
+}
+
+NTSTATUS
+virt2phys_init(void)
+{
+	g_lock = ExAllocatePoolZero(NonPagedPool, sizeof(*g_lock), 'gp2v');
+	if (g_lock == NULL)
+		return STATUS_INSUFFICIENT_RESOURCES;
+
+	InitializeListHead(&g_processes);
+
+	return STATUS_SUCCESS;
+}
+
+void
+virt2phys_cleanup(void)
+{
+	PLIST_ENTRY node, next;
+	struct virt2phys_process *process;
+	KIRQL irql;
+
+	KeAcquireSpinLock(g_lock, &irql);
+	for (node = g_processes.Flink; node != &g_processes; node = next) {
+		next = node->Flink;
+		process = CONTAINING_RECORD(node, struct virt2phys_process, next);
+		RemoveEntryList(&process->next);
+		KeReleaseSpinLock(g_lock, irql);
+
+		virt2phys_process_free(process, FALSE);
+
+		KeAcquireSpinLock(g_lock, &irql);
+	}
+	KeReleaseSpinLock(g_lock, irql);
+}
+
+static struct virt2phys_process *
+virt2phys_process_detach(HANDLE process_id)
+{
+	struct virt2phys_process *process;
+
+	process = virt2phys_process_find(process_id);
+	if (process != NULL)
+		RemoveEntryList(&process->next);
+	return process;
+}
+
+void
+virt2phys_process_cleanup(HANDLE process_id)
+{
+	struct virt2phys_process *process;
+	KIRQL irql;
+
+	KeAcquireSpinLock(g_lock, &irql);
+	process = virt2phys_process_detach(process_id);
+	KeReleaseSpinLock(g_lock, irql);
+
+	if (process != NULL)
+		virt2phys_process_free(process, TRUE);
+}
+
+static struct virt2phys_block *
+virt2phys_find_block(HANDLE process_id, void *virt,
+	struct virt2phys_process **process)
+{
+	PLIST_ENTRY node;
+	struct virt2phys_process *cur;
+
+	for (node = g_processes.Flink; node != &g_processes;
+			node = node->Flink) {
+		cur = CONTAINING_RECORD(node, struct virt2phys_process, next);
+		if (cur->id == process_id) {
+			*process = cur;
+			return virt2phys_process_find_block(cur, virt);
+		}
+	}
+
+	*process = NULL;
+	return NULL;
+}
+
+static BOOLEAN
+virt2phys_exceeeds(LONG64 count, ULONG64 limit)
+{
+	return limit > 0 && count > (LONG64)limit;
+}
+
+static BOOLEAN
+virt2phys_add_block(struct virt2phys_process *process,
+	struct virt2phys_block *block)
+{
+	struct virt2phys_process *existing;
+
+	existing = virt2phys_process_find(process->id);
+	if (existing == NULL)
+		InsertHeadList(&g_processes, &process->next);
+	else
+		process = existing;
+
+	PushEntryList(&process->blocks, &block->next);
+
+	return existing != NULL;
+}
+
+static NTSTATUS
+virt2phys_query_memory(void *virt, void **base, size_t *size)
+{
+	MEMORY_BASIC_INFORMATION info;
+	SIZE_T info_size;
+	NTSTATUS status;
+
+	status = ZwQueryVirtualMemory(
+		ZwCurrentProcess(), virt, MemoryBasicInformation,
+		&info, sizeof(info), &info_size);
+	if (NT_SUCCESS(status)) {
+		*base = info.AllocationBase;
+		*size = info.RegionSize;
+	}
+	return status;
+}
+
+static BOOLEAN
+virt2phys_is_contiguous(PMDL mdl)
+{
+	PPFN_NUMBER pfn;
+	size_t i, pfn_count;
+
+	pfn = MmGetMdlPfnArray(mdl);
+	pfn_count = ADDRESS_AND_SIZE_TO_SPAN_PAGES(
+		MmGetMdlVirtualAddress(mdl), MmGetMdlByteCount(mdl));
+	for (i = 1; i < pfn_count; i++) {
+		if (pfn[i] != pfn[i - 1] + 1)
+			return FALSE;
+	}
+	return TRUE;
+}
+
+static NTSTATUS
+virt2phys_check_memory(PMDL mdl)
+{
+	MEMORY_BASIC_INFORMATION info;
+	SIZE_T info_size;
+	PVOID virt;
+	size_t size;
+	NTSTATUS status;
+
+	if (!virt2phys_is_contiguous(mdl))
+		return STATUS_UNSUCCESSFUL;
+
+	virt = MmGetMdlVirtualAddress(mdl);
+	size = MmGetMdlByteCount(mdl);
+	status = ZwQueryVirtualMemory(
+		ZwCurrentProcess(), virt, MemoryBasicInformation,
+		&info, sizeof(info), &info_size);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	if (info.AllocationBase != virt || info.RegionSize != size)
+		return STATUS_UNSUCCESSFUL;
+	if (info.State != MEM_COMMIT)
+		return STATUS_UNSUCCESSFUL;
+	if (info.Type != MEM_PRIVATE)
+		return STATUS_UNSUCCESSFUL;
+	return status;
+}
+
+static NTSTATUS
+virt2phys_lock_memory(void *virt, size_t size, PMDL *mdl)
+{
+	*mdl = IoAllocateMdl(virt, (ULONG)size, FALSE, FALSE, NULL);
+	if (*mdl == NULL)
+		return STATUS_INSUFFICIENT_RESOURCES;
+
+	__try {
+		/* Future memory usage is unknown, declare RW access. */
+		MmProbeAndLockPages(*mdl, UserMode, IoModifyAccess);
+	}
+	__except (EXCEPTION_EXECUTE_HANDLER) {
+		IoFreeMdl(*mdl);
+		*mdl = NULL;
+		return STATUS_UNSUCCESSFUL;
+	}
+	return STATUS_SUCCESS;
+}
+
+static VOID
+virt2phys_unlock_memory(PMDL mdl)
+{
+	MmUnlockPages(mdl);
+	IoFreeMdl(mdl);
+}
+
+NTSTATUS
+virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys)
+{
+	PMDL mdl;
+	HANDLE process_id;
+	void *base;
+	size_t size;
+	struct virt2phys_process *process;
+	struct virt2phys_block *block;
+	BOOLEAN created, tracked;
+	KIRQL irql;
+	NTSTATUS status;
+
+	process_id = PsGetCurrentProcessId();
+
+	status = virt2phys_query_memory(virt, &base, &size);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	KeAcquireSpinLock(g_lock, &irql);
+	block = virt2phys_find_block(process_id, base, &process);
+	KeReleaseSpinLock(g_lock, irql);
+
+	/* Don't lock the same memory twice. */
+	if (block != NULL) {
+		*phys = virt2phys_block_translate(block, virt);
+		return STATUS_SUCCESS;
+	}
+
+	status = virt2phys_lock_memory(base, size, &mdl);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_check_memory(mdl);
+	if (!NT_SUCCESS(status)) {
+		virt2phys_unlock_memory(mdl);
+		return status;
+	}
+
+	block = virt2phys_block_create(mdl);
+	if (block == NULL) {
+		virt2phys_unlock_memory(mdl);
+		return STATUS_INSUFFICIENT_RESOURCES;
+	}
+
+	created = FALSE;
+	if (process == NULL) {
+		process = virt2phys_process_create(process_id);
+		if (process == NULL) {
+			virt2phys_block_free(block, TRUE);
+			return STATUS_INSUFFICIENT_RESOURCES;
+		}
+		created = TRUE;
+	}
+
+	KeAcquireSpinLock(g_lock, &irql);
+	tracked = virt2phys_add_block(process, block);
+	KeReleaseSpinLock(g_lock, irql);
+
+	/* Same process has been added concurrently, block attached to it. */
+	if (tracked && created)
+		virt2phys_process_free(process, FALSE);
+
+	*phys = virt2phys_block_translate(block, virt);
+	return STATUS_SUCCESS;
+}
diff --git a/windows/virt2phys/virt2phys_logic.h b/windows/virt2phys/virt2phys_logic.h
new file mode 100644
index 0000000..1582206
--- /dev/null
+++ b/windows/virt2phys/virt2phys_logic.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Dmitry Kozlyuk
+ */
+
+#ifndef VIRT2PHYS_LOGIC_H
+#define VIRT2PHYS_LOGIC_H
+
+/**
+ * Initialize internal data structures.
+ */
+NTSTATUS virt2phys_init(void);
+
+/**
+ * Free memory allocated for internal data structures.
+ * Do not unlock memory so that it's not paged even if driver is unloaded
+ * when an application still uses this memory.
+ */
+void virt2phys_cleanup(void);
+
+/**
+ * Unlock all tracked memory blocks of a process.
+ * Free memory allocated for tracking of the process.
+ */
+void virt2phys_process_cleanup(HANDLE process_id);
+
+/**
+ * Lock current process memory region containing @p virt
+ * and get physical address corresponding to @p virt.
+ */
+NTSTATUS virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys);
+
+#endif /* VIRT2PHYS_LOGIC_H */
-- 
2.29.3