[dpdk-dev] [kmods PATCH 2/3] 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>
---
 windows/virt2phys/virt2phys.c               |  75 +++--
 windows/virt2phys/virt2phys.vcxproj         |   2 +
 windows/virt2phys/virt2phys.vcxproj.filters |   8 +-
 windows/virt2phys/virt2phys_logic.c         | 304 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  32 +++
 5 files changed, 394 insertions(+), 27 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..0c05fe3 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,42 @@ 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));
+		return status;
 	}
 
+	status = virt2phys_init();
+	if (!NT_SUCCESS(status))
+		return status;
+
+	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 +70,6 @@ virt2phys_driver_EvtDeviceAdd(
 
 	UNREFERENCED_PARAMETER(driver);
 
-	PAGED_CODE();
-
 	WdfDeviceInitSetIoType(
 		init, WdfDeviceIoNeither);
 	WdfDeviceInitSetIoInCallerContextCallback(
@@ -57,15 +79,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 +93,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 +103,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 +122,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 +129,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 e5ce5fe..294f086 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..6b65d71 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>
+</Project>
\ No newline at end of file
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
new file mode 100644
index 0000000..155d34f
--- /dev/null
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -0,0 +1,304 @@
+/* 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 = ExAllocatePool(NonPagedPool, sizeof(*block));
+	if (block != NULL) {
+		RtlZeroMemory(block, sizeof(*block));
+		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 struct virt2phys_process *
+virt2phys_process_create(HANDLE process_id)
+{
+	struct virt2phys_process *process;
+
+	process = ExAllocatePool(NonPagedPool, sizeof(*process));
+	if (process != NULL) {
+		RtlZeroMemory(process, sizeof(*process));
+		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 BOOLEAN
+virt2phys_process_has_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 TRUE;
+	}
+	return FALSE;
+}
+
+NTSTATUS
+virt2phys_init(void)
+{
+	g_lock = ExAllocatePool(NonPagedPool, sizeof(*g_lock));
+	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 BOOLEAN
+virt2phys_has_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_has_block(cur, virt);
+		}
+	}
+
+	*process = NULL;
+	return FALSE;
+}
+
+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))
+		return status;
+	if (info.State == MEM_FREE)
+		return STATUS_MEMORY_NOT_ALLOCATED;
+
+	*base = info.AllocationBase;
+	*size = info.RegionSize;
+	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 locked, 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);
+	locked = virt2phys_has_block(process_id, base, &process);
+	KeReleaseSpinLock(g_lock, irql);
+
+	/* Don't lock the same memory twice. */
+	if (locked) {
+		*phys = MmGetPhysicalAddress(virt);
+		return STATUS_SUCCESS;
+	}
+
+	status = virt2phys_lock_memory(base, size, &mdl);
+	if (!NT_SUCCESS(status))
+		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 = MmGetPhysicalAddress(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