[dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue

DPDK patches and discussions
 help / color / mirror / Atom feed

* [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue
@ 2021-05-01 17:18 Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 1/3] windows/virt2phys: use local time for signing Dmitry Kozlyuk
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-01 17:18 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

Physical addresses exposed by virt2phys driver could become pageable.
This presents stability and security issues that prevent Microsoft
from signing virt2phys, because a signed driver would be trusted
by all end-user machines.

Ensure that memory for which physical addresses are exposed by
virt2phys is non-pageable at least for the lifetime of the process.
As virt2phys code grows, make its development and debugging easier.

There are other known issues that come from using PA and accessing DMA
from userspace. They are not related to virt2phys par se. It is planned
to address them later by enabling the use of IOMMU for DPDK on Windows.

Dmitry Kozlyuk (3):
  windows/virt2phys: use local time for signing
  windows/virt2phys: do not expose pageable physical addresses
  windows/virt2phys: add tracing

 windows/virt2phys/virt2phys.c               |  89 ++++--
 windows/virt2phys/virt2phys.vcxproj         |   8 +-
 windows/virt2phys/virt2phys.vcxproj.filters |  11 +-
 windows/virt2phys/virt2phys_logic.c         | 312 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  32 ++
 windows/virt2phys/virt2phys_trace.h         |  49 +++
 6 files changed, 472 insertions(+), 29 deletions(-)
 create mode 100644 windows/virt2phys/virt2phys_logic.c
 create mode 100644 windows/virt2phys/virt2phys_logic.h
 create mode 100644 windows/virt2phys/virt2phys_trace.h

-- 
2.29.3

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH 1/3] windows/virt2phys: use local time for signing
  2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
@ 2021-05-01 17:18 ` Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 2/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-01 17:18 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

Inf2Cat utility for signing drivers considers time in UTC by default
and aborts with "date in the future" error when developing in
positive-offset timezones. Set "Use Local Time" flag to fix it.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 windows/virt2phys/virt2phys.vcxproj | 1 +
 1 file changed, 1 insertion(+)

diff --git a/windows/virt2phys/virt2phys.vcxproj b/windows/virt2phys/virt2phys.vcxproj
index c86cc9b..e5ce5fe 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -133,6 +133,7 @@
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
+    <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH 2/3] windows/virt2phys: do not expose pageable physical addresses
  2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 1/3] windows/virt2phys: use local time for signing Dmitry Kozlyuk
@ 2021-05-01 17:18 ` Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-01 17:18 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH 3/3] windows/virt2phys: add tracing
  2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 1/3] windows/virt2phys: use local time for signing Dmitry Kozlyuk
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 2/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
@ 2021-05-01 17:18 ` Dmitry Kozlyuk
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
  2021-06-29  5:16 ` [dpdk-dev] [kmods PATCH " Ranjit Menon
  4 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-01 17:18 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

WPP tracing [1] allows kernel drivers to print logs that can be viewed
without attaching a debugger to the running system. Traces are colelcted
only when enabled. Instrument virt2phys with traces:
* ERROR:   failures that prevent the driver from working.
* WARNING: incorrect calls to the driver.
* INFO:    starting or completing operations with memory.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/wpp-software-tracing

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 windows/virt2phys/virt2phys.c               | 18 +++++++-
 windows/virt2phys/virt2phys.vcxproj         |  5 ++-
 windows/virt2phys/virt2phys.vcxproj.filters |  3 ++
 windows/virt2phys/virt2phys_logic.c         |  8 ++++
 windows/virt2phys/virt2phys_trace.h         | 49 +++++++++++++++++++++
 5 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100644 windows/virt2phys/virt2phys_trace.h

diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index 0c05fe3..54f4bc0 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -8,6 +8,8 @@
 
 #include "virt2phys.h"
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys.tmh"
 
 DRIVER_INITIALIZE DriverEntry;
 EVT_WDF_DRIVER_UNLOAD virt2phys_driver_unload;
@@ -46,6 +48,8 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	if (!NT_SUCCESS(status))
 		return status;
 
+	WPP_INIT_TRACING(driver_object, registry_path);
+
 	return status;
 }
 
@@ -53,11 +57,11 @@ _Use_decl_annotations_
 VOID
 virt2phys_driver_unload(WDFDRIVER driver)
 {
-	UNREFERENCED_PARAMETER(driver);
-
 	PsSetCreateProcessNotifyRoutine(virt2phys_on_process_event, TRUE);
 
 	virt2phys_cleanup();
+
+	WPP_CLEANUP(WdfDriverWdmGetDriverObject(driver));
 }
 
 _Use_decl_annotations_
@@ -79,12 +83,15 @@ virt2phys_driver_EvtDeviceAdd(WDFDRIVER driver, PWDFDEVICE_INIT init)
 
 	status = WdfDeviceCreate(&init, &attributes, &device);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDriverCreate() = %!STATUS!", status);
 		return status;
 	}
 
 	status = WdfDeviceCreateDeviceInterface(
 		device, &GUID_DEVINTERFACE_VIRT2PHYS, NULL);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDeviceCreateDeviceInterface() = %!STATUS!",
+			status);
 		return status;
 	}
 
@@ -109,12 +116,14 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	WdfRequestGetParameters(request, &params);
 
 	if (params.Type != WdfRequestTypeDeviceControl) {
+		TraceWarning("Bogus IO request type %lu", params.Type);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
 
 	code = params.Parameters.DeviceIoControl.IoControlCode;
 	if (code != IOCTL_VIRT2PHYS_TRANSLATE) {
+		TraceWarning("Bogus IO control code %lx", code);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
@@ -122,6 +131,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveInputBuffer(
 			request, sizeof(*virt), (PVOID *)&virt, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving input buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -129,6 +139,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveOutputBuffer(
 		request, sizeof(*phys), (PVOID *)&phys, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving output buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -136,6 +147,9 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = virt2phys_translate(*virt, phys);
 	if (NT_SUCCESS(status))
 		WdfRequestSetInformation(request, sizeof(*phys));
+
+	TraceInfo("Translate %p to %llx: %!STATUS!",
+		virt, phys->QuadPart, status);
 	WdfRequestComplete(request, status);
 }
 
diff --git a/windows/virt2phys/virt2phys.vcxproj b/windows/virt2phys/virt2phys.vcxproj
index 294f086..a3e18dc 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -41,6 +41,7 @@
   <ItemGroup>
     <ClInclude Include="virt2phys.h" />
     <ClInclude Include="virt2phys_logic.h" />
+    <ClInclude Include="virt2phys_trace.h" />
   </ItemGroup>
   <ItemGroup>
     <Inf Include="virt2phys.inf" />
@@ -170,9 +171,9 @@
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
-      <WppEnabled>false</WppEnabled>
+      <WppEnabled>true</WppEnabled>
       <WppRecorderEnabled>true</WppRecorderEnabled>
-      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
+      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
     </ClCompile>
     <Link>
diff --git a/windows/virt2phys/virt2phys.vcxproj.filters b/windows/virt2phys/virt2phys.vcxproj.filters
index 6b65d71..b2f6776 100644
--- a/windows/virt2phys/virt2phys.vcxproj.filters
+++ b/windows/virt2phys/virt2phys.vcxproj.filters
@@ -30,6 +30,9 @@
     <ClInclude Include="virt2phys_logic.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="virt2phys_trace.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virt2phys.c">
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
index 155d34f..7d5bf02 100644
--- a/windows/virt2phys/virt2phys_logic.c
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -6,6 +6,8 @@
 #include <ntddk.h>
 
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys_logic.tmh"
 
 struct virt2phys_process {
 	HANDLE id;
@@ -37,6 +39,8 @@ virt2phys_block_create(PMDL mdl)
 static void
 virt2phys_block_free(struct virt2phys_block *block, BOOLEAN unmap)
 {
+	TraceInfo("VA = %p, unmap = %!bool!", block->mdl->StartVa, unmap);
+
 	if (unmap)
 		MmUnlockPages(block->mdl);
 
@@ -63,6 +67,8 @@ virt2phys_process_free(struct virt2phys_process *process, BOOLEAN unmap)
 	PSINGLE_LIST_ENTRY node;
 	struct virt2phys_block *block;
 
+	TraceInfo("ID = %p, unmap = %!bool!", process->id, unmap);
+
 	node = process->blocks.Next;
 	while (node != NULL) {
 		block = CONTAINING_RECORD(node, struct virt2phys_block, next);
@@ -185,6 +191,8 @@ virt2phys_add_block(struct virt2phys_process *process,
 {
 	struct virt2phys_process *existing;
 
+	TraceInfo("ID = %p, VA = %p", process->id, block->mdl->StartVa);
+
 	existing = virt2phys_process_find(process->id);
 	if (existing == NULL)
 		InsertHeadList(&g_processes, &process->next);
diff --git a/windows/virt2phys/virt2phys_trace.h b/windows/virt2phys/virt2phys_trace.h
new file mode 100644
index 0000000..ebd7618
--- /dev/null
+++ b/windows/virt2phys/virt2phys_trace.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Dmitry Kozlyuk
+ */
+
+/* Tracing GUID: C5C835BB-5CFB-4757-B1D4-9DD74662E212 */
+#define WPP_CONTROL_GUIDS \
+	WPP_DEFINE_CONTROL_GUID( \
+		VIRT2PHYS_TRACE_GUID, (C5C835BB,5CFB,4757,B1D4,9DD74662E212), \
+		WPP_DEFINE_BIT(TRACE_GENERAL))
+
+#define WPP_FLAG_LEVEL_LOGGER(flag, level) \
+    WPP_LEVEL_LOGGER(flag)
+
+#define WPP_FLAG_LEVEL_ENABLED(flag, level) \
+		(WPP_LEVEL_ENABLED(flag) && \
+			WPP_CONTROL(WPP_BIT_ ## flag).Level >= level)
+
+#define WPP_LEVEL_FLAGS_LOGGER(lvl,flags) \
+	   WPP_LEVEL_LOGGER(flags)
+
+#define WPP_LEVEL_FLAGS_ENABLED(lvl, flags) \
+		(WPP_LEVEL_ENABLED(flags) && \
+			WPP_CONTROL(WPP_BIT_ ## flags).Level >= lvl)
+
+/*
+ * WPP orders static parameters before dynamic parameters.
+ * To support trace functions defined below which sets FLAGS and LEVEL,
+ * a custom macro must be defined to reorder the arguments
+ * to what the .tpl configuration file expects.
+ */
+#define WPP_RECORDER_FLAGS_LEVEL_ARGS(flags, lvl) \
+		WPP_RECORDER_LEVEL_FLAGS_ARGS(lvl, flags)
+#define WPP_RECORDER_FLAGS_LEVEL_FILTER(flags, lvl) \
+		WPP_RECORDER_LEVEL_FLAGS_FILTER(lvl, flags)
+
+/*
+begin_wpp config
+
+USEPREFIX(TraceError, "[%!FUNC!] ");
+FUNC TraceError{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_ERROR}(MSG, ...);
+
+USEPREFIX(TraceWarning, "[%!FUNC!] ");
+FUNC TraceWarning{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_WARNING}(MSG, ...);
+
+USEPREFIX(TraceInfo, "[%!FUNC!] ");
+FUNC TraceInfo{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_INFORMATION}(MSG, ...);
+
+end_wpp
+*/
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue
  2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
                   ` (2 preceding siblings ...)
  2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
@ 2021-05-26 21:01 ` Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 1/4] windows/virt2phys: add PnpLockdown directive Dmitry Kozlyuk
                     ` (5 more replies)
  2021-06-29  5:16 ` [dpdk-dev] [kmods PATCH " Ranjit Menon
  4 siblings, 6 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:01 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

Physical addresses exposed by virt2phys driver could become pageable.
This presents stability and security issues that prevent Microsoft
from signing virt2phys, because a signed driver would be trusted
by all end-user machines.

Ensure that memory for which physical addresses are exposed by
virt2phys is non-pageable at least for the lifetime of the process.
As virt2phys code grows, make its development and debugging easier.

There are other known issues that come from using PA and accessing DMA
from userspace. They are not related to virt2phys par se. It is planned
to address them later by enabling the use of IOMMU for DPDK on Windows.

v2:
    * Following ofline review by DmitryM:
      - Add comment explaining tracking approach for validation team.
      - Replace deprecated allocation API calls.
      - Check properties of locked memory (see docs in patch 3/4).
      - Add configurable limits for tracked processes and memory.
    * Add end-user documentation.
    * Drop patch for Inf2Cat settings UseLocalTime=true:
      the issue it resolves originated from development VM.
    * Add PnpLockdown=1 patch.

Dmitry Kozlyuk (4):
  windows/virt2phys: add PnpLockdown directive
  windows/virt2phys: do not expose pageable physical addresses
  windows/virt2phys: add limits against resource exhaustion
  windows/virt2phys: add tracing

 windows/virt2phys/README.md                 |  38 ++
 windows/virt2phys/virt2phys.c               | 173 ++++++--
 windows/virt2phys/virt2phys.inf             |   1 +
 windows/virt2phys/virt2phys.vcxproj         |   7 +-
 windows/virt2phys/virt2phys.vcxproj.filters |   9 +
 windows/virt2phys/virt2phys_logic.c         | 415 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  39 ++
 windows/virt2phys/virt2phys_trace.h         |  50 +++
 8 files changed, 701 insertions(+), 31 deletions(-)
 create mode 100644 windows/virt2phys/README.md
 create mode 100644 windows/virt2phys/virt2phys_logic.c
 create mode 100644 windows/virt2phys/virt2phys_logic.h
 create mode 100644 windows/virt2phys/virt2phys_trace.h

-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v2 1/4] windows/virt2phys: add PnpLockdown directive
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
@ 2021-05-26 21:01   ` Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 2/4] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:01 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

WDK for Windows 10, version 2004 emits a warning suggesting to add
PnpLockdown directive to INI [1]. Add it since virt2phys has no
potential use-cases that require modifications to driver files.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/install/inf-version-section

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 windows/virt2phys/virt2phys.inf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/windows/virt2phys/virt2phys.inf b/windows/virt2phys/virt2phys.inf
index e35765e..7ca42f4 100644
--- a/windows/virt2phys/virt2phys.inf
+++ b/windows/virt2phys/virt2phys.inf
@@ -8,6 +8,7 @@ ClassGuid = {78A1C341-4539-11d3-B88D-00C04FAD5171}
 Provider = %ManufacturerName%
 CatalogFile = virt2phys.cat
 DriverVer =
+PnpLockdown = 1
 
 [DestinationDirs]
 DefaultDestDir = 12
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v2 2/4] windows/virt2phys: do not expose pageable physical addresses
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 1/4] windows/virt2phys: add PnpLockdown directive Dmitry Kozlyuk
@ 2021-05-26 21:01   ` Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 3/4] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:01 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v2 3/4] windows/virt2phys: add limits against resource exhaustion
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 1/4] windows/virt2phys: add PnpLockdown directive Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 2/4] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
@ 2021-05-26 21:01   ` Dmitry Kozlyuk
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing Dmitry Kozlyuk
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:01 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

Tracked processes are enumerated in process creation/termination
callback, making it O(N = number of tracked processes). A malicious user
could cause system-wide slowdown of process termination by making just one
call to virt2phys from many processes. Limit the number of tracked
processes by `ProcessCountLimit` items.

Any process with access to virt2phys could exhaust RAM for all the
system by locking it. It can also exhaust RAM needed for other processes
with access to virt2phys. Limit amount of memory locked by each tracked
process with `ProcessMemoryLimitMB`.

All limits are read from registry at driver load.
Each limit can be turned off by setting it to 0.
Document limits and their non-zero defaults for administrators.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 windows/virt2phys/README.md         | 38 +++++++++++++++
 windows/virt2phys/virt2phys.c       | 73 ++++++++++++++++++++++++++++-
 windows/virt2phys/virt2phys_logic.c | 46 +++++++++++++-----
 windows/virt2phys/virt2phys_logic.h |  9 +++-
 4 files changed, 152 insertions(+), 14 deletions(-)
 create mode 100644 windows/virt2phys/README.md

diff --git a/windows/virt2phys/README.md b/windows/virt2phys/README.md
new file mode 100644
index 0000000..04012ff
--- /dev/null
+++ b/windows/virt2phys/README.md
@@ -0,0 +1,38 @@
+Virtual to Physical Address Translator
+======================================
+
+Purpose and Operation
+---------------------
+
+``virt2phys`` driver allows user-mode processes to obtain physical address
+of a given virtual address in their address space.
+Virtual addresses must belong to regions from process private working set.
+These regions must be physically contiguous.
+The driver ensures that memory regions with translated addresses
+are not swapped out as long as the process has access to this memory.
+
+It is not safe to administratively unload the driver
+while there are processes that have used virt2phys to translate addresses.
+Doing so will permanently leak RAM occupied by all memory regions
+that contain translated addresses.
+Terminate all such processes before unloading the driver.
+
+Configuration
+-------------
+
+``virt2phys`` is configured at loading time via registry key
+``HKLM\SYSTEM\ControlSet001\Services\virt2phys\Parameters``.
+
+* ``ProcessCountLimit`` (default 16)
+
+  Maximum number of processes that can have access to memory regions
+  with translated addresses. When this limit is reached, no more processes
+  can translate addresses using ``virt2phys``. Large number of tracked
+  processes may slow down system operation. Set limit to 0 to disable it.
+
+* ``ProcessMemoryLimitMB`` (default 16384, i.e. 16 GB)
+
+  Maximum amount of memory in all regions that contain translated addresses,
+  total per process. When this limit is reached, the process can not translate
+  addresses from new regions. Large values can cause RAM exhaustion.
+  Set limit to 0 to disable it.
\ No newline at end of file
diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index bf0c300..44204c9 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -14,15 +14,22 @@ 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 NTSTATUS virt2phys_load_params(
+	WDFDRIVER driver, struct virt2phys_params *params);
 static VOID virt2phys_on_process_event(
 	HANDLE parent_id, HANDLE process_id, BOOLEAN create);
 
+static const ULONG PROCESS_COUNT_LIMIT_DEF = 1 << 4;
+static const ULONG PROCESS_MEMORY_LIMIT_DEF = 16 * (1 << 10); /* MB */
+
 _Use_decl_annotations_
 NTSTATUS
 DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 {
 	WDF_DRIVER_CONFIG config;
 	WDF_OBJECT_ATTRIBUTES attributes;
+	WDFDRIVER driver;
+	struct virt2phys_params params;
 	NTSTATUS status;
 
 	PAGED_CODE();
@@ -32,11 +39,15 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	config.EvtDriverUnload = virt2phys_driver_unload;
 	status = WdfDriverCreate(
 		driver_object, registry_path,
-		&attributes, &config, WDF_NO_HANDLE);
+		&attributes, &config, &driver);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_load_params(driver, &params);
 	if (!NT_SUCCESS(status))
 		return status;
 
-	status = virt2phys_init();
+	status = virt2phys_init(&params);
 	if (!NT_SUCCESS(status))
 		return status;
 
@@ -58,6 +69,64 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	return status;
 }
 
+static NTSTATUS
+virt2phys_read_param(WDFKEY key, PCUNICODE_STRING name, ULONG *value,
+	ULONG def)
+{
+	NTSTATUS status;
+
+	status = WdfRegistryQueryULong(key, name, value);
+	if (status == STATUS_OBJECT_NAME_NOT_FOUND) {
+		*value = def;
+		status = STATUS_SUCCESS;
+	}
+	return status;
+}
+
+static NTSTATUS
+virt2phys_read_mb(WDFKEY key, PCUNICODE_STRING name, ULONG64 *bytes,
+	ULONG def_mb)
+{
+	ULONG mb;
+	NTSTATUS status;
+
+	status = virt2phys_read_param(key, name, &mb, def_mb);
+	if (NT_SUCCESS(status))
+		*bytes = (ULONG64)mb * (1ULL << 20);
+	return status;
+}
+
+static NTSTATUS
+virt2phys_load_params(WDFDRIVER driver, struct virt2phys_params *params)
+{
+	static DECLARE_CONST_UNICODE_STRING(
+		process_count_limit, L"ProcessCountLimit");
+	static DECLARE_CONST_UNICODE_STRING(
+		process_memory_limit, L"ProcessMemoryLimitMB");
+
+	WDFKEY key;
+	NTSTATUS status;
+
+	status = WdfDriverOpenParametersRegistryKey(
+		driver, KEY_READ, WDF_NO_OBJECT_ATTRIBUTES, &key);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_read_param(key, &process_count_limit,
+		&params->process_count_limit, PROCESS_COUNT_LIMIT_DEF);
+	if (!NT_SUCCESS(status))
+		goto cleanup;
+
+	status = virt2phys_read_mb(key, &process_memory_limit,
+		&params->process_memory_limit, PROCESS_MEMORY_LIMIT_DEF);
+	if (!NT_SUCCESS(status))
+		goto cleanup;
+
+cleanup:
+	WdfRegistryClose(key);
+	return status;
+}
+
 _Use_decl_annotations_
 VOID
 virt2phys_driver_unload(WDFDRIVER driver)
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
index a27802c..37b4dd4 100644
--- a/windows/virt2phys/virt2phys_logic.c
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -11,6 +11,7 @@ struct virt2phys_process {
 	HANDLE id;
 	LIST_ENTRY next;
 	SINGLE_LIST_ENTRY blocks;
+	ULONG64 memory;
 };
 
 struct virt2phys_block {
@@ -18,7 +19,9 @@ struct virt2phys_block {
 	SINGLE_LIST_ENTRY next;
 };
 
+static struct virt2phys_params g_params;
 static LIST_ENTRY g_processes;
+static LONG g_process_count;
 static PKSPIN_LOCK g_lock;
 
 struct virt2phys_block *
@@ -112,7 +115,7 @@ virt2phys_process_find_block(struct virt2phys_process *process, PVOID virt)
 }
 
 NTSTATUS
-virt2phys_init(void)
+virt2phys_init(const struct virt2phys_params *params)
 {
 	g_lock = ExAllocatePoolZero(NonPagedPool, sizeof(*g_lock), 'gp2v');
 	if (g_lock == NULL)
@@ -120,6 +123,7 @@ virt2phys_init(void)
 
 	InitializeListHead(&g_processes);
 
+	g_params = *params;
 	return STATUS_SUCCESS;
 }
 
@@ -165,8 +169,10 @@ virt2phys_process_cleanup(HANDLE process_id)
 	process = virt2phys_process_detach(process_id);
 	KeReleaseSpinLock(g_lock, irql);
 
-	if (process != NULL)
+	if (process != NULL) {
 		virt2phys_process_free(process, TRUE);
+		InterlockedDecrement(&g_process_count);
+	}
 }
 
 static struct virt2phys_block *
@@ -195,21 +201,38 @@ virt2phys_exceeeds(LONG64 count, ULONG64 limit)
 	return limit > 0 && count > (LONG64)limit;
 }
 
-static BOOLEAN
+static NTSTATUS
 virt2phys_add_block(struct virt2phys_process *process,
-	struct virt2phys_block *block)
+	struct virt2phys_block *block, BOOLEAN *process_exists)
 {
 	struct virt2phys_process *existing;
+	size_t size;
 
 	existing = virt2phys_process_find(process->id);
-	if (existing == NULL)
+	*process_exists = existing != NULL;
+	if (existing == NULL) {
+		/*
+		 * This check is done with the lock held so that's no race.
+		 * Increment below must be atomic however,
+		 * because decrement is done without holding the lock.
+		 */
+		if (virt2phys_exceeeds(g_process_count + 1,
+				g_params.process_count_limit))
+			return STATUS_QUOTA_EXCEEDED;
+
 		InsertHeadList(&g_processes, &process->next);
-	else
+		InterlockedIncrement(&g_process_count);
+	} else
 		process = existing;
 
-	PushEntryList(&process->blocks, &block->next);
+	size = MmGetMdlByteCount(block->mdl);
+	if (virt2phys_exceeeds(process->memory + size,
+			g_params.process_memory_limit))
+		return STATUS_QUOTA_EXCEEDED;
 
-	return existing != NULL;
+	PushEntryList(&process->blocks, &block->next);
+	process->memory += size;
+	return STATUS_SUCCESS;
 }
 
 static NTSTATUS
@@ -356,13 +379,14 @@ virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys)
 	}
 
 	KeAcquireSpinLock(g_lock, &irql);
-	tracked = virt2phys_add_block(process, block);
+	status = virt2phys_add_block(process, block, &tracked);
 	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;
+	if (NT_SUCCESS(status))
+		*phys = virt2phys_block_translate(block, virt);
+	return status;
 }
diff --git a/windows/virt2phys/virt2phys_logic.h b/windows/virt2phys/virt2phys_logic.h
index 1582206..c8255f9 100644
--- a/windows/virt2phys/virt2phys_logic.h
+++ b/windows/virt2phys/virt2phys_logic.h
@@ -5,10 +5,17 @@
 #ifndef VIRT2PHYS_LOGIC_H
 #define VIRT2PHYS_LOGIC_H
 
+struct virt2phys_params {
+	/** Maximum number of tracked processes (0 = unlimited). */
+	ULONG process_count_limit;
+	/** Maximum amount of memory locked by a process (0 = unlimited). */
+	ULONG64 process_memory_limit;
+};
+
 /**
  * Initialize internal data structures.
  */
-NTSTATUS virt2phys_init(void);
+NTSTATUS virt2phys_init(const struct virt2phys_params *params);
 
 /**
  * Free memory allocated for internal data structures.
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
                     ` (2 preceding siblings ...)
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 3/4] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
@ 2021-05-26 21:01   ` Dmitry Kozlyuk
  2021-09-30 22:07     ` Menon, Ranjit
  2021-06-23  7:13   ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Thomas Monjalon
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
  5 siblings, 1 reply; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:01 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly, Dmitry Kozlyuk

WPP tracing [1] allows kernel drivers to print logs that can be viewed
without attaching a debugger to the running system. Traces are colelcted
only when enabled. Instrument virt2phys with traces:
* ERROR:   failures that prevent the driver from working.
* WARNING: incorrect calls to the driver.
* INFO:    starting or completing operations with memory.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/wpp-software-tracing

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
Comment at the bottom of virt2phys_trace.h is consumed by WPP code
generator, which is the reason it has no leading asterisks.

 windows/virt2phys/virt2phys.c               | 18 +++++++-
 windows/virt2phys/virt2phys.vcxproj         |  5 ++-
 windows/virt2phys/virt2phys.vcxproj.filters |  3 ++
 windows/virt2phys/virt2phys_logic.c         | 35 ++++++++++++---
 windows/virt2phys/virt2phys_trace.h         | 50 +++++++++++++++++++++
 5 files changed, 101 insertions(+), 10 deletions(-)
 create mode 100644 windows/virt2phys/virt2phys_trace.h

diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index 44204c9..f4d5298 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -8,6 +8,8 @@
 
 #include "virt2phys.h"
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys.tmh"
 
 DRIVER_INITIALIZE DriverEntry;
 EVT_WDF_DRIVER_UNLOAD virt2phys_driver_unload;
@@ -66,6 +68,8 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	if (!NT_SUCCESS(status))
 		return status;
 
+	WPP_INIT_TRACING(driver_object, registry_path);
+
 	return status;
 }
 
@@ -131,11 +135,11 @@ _Use_decl_annotations_
 VOID
 virt2phys_driver_unload(WDFDRIVER driver)
 {
-	UNREFERENCED_PARAMETER(driver);
-
 	PsSetCreateProcessNotifyRoutine(virt2phys_on_process_event, TRUE);
 
 	virt2phys_cleanup();
+
+	WPP_CLEANUP(WdfDriverWdmGetDriverObject(driver));
 }
 
 _Use_decl_annotations_
@@ -157,12 +161,15 @@ virt2phys_driver_EvtDeviceAdd(WDFDRIVER driver, PWDFDEVICE_INIT init)
 
 	status = WdfDeviceCreate(&init, &attributes, &device);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDriverCreate() = %!STATUS!", status);
 		return status;
 	}
 
 	status = WdfDeviceCreateDeviceInterface(
 		device, &GUID_DEVINTERFACE_VIRT2PHYS, NULL);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDeviceCreateDeviceInterface() = %!STATUS!",
+			status);
 		return status;
 	}
 
@@ -187,12 +194,14 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	WdfRequestGetParameters(request, &params);
 
 	if (params.Type != WdfRequestTypeDeviceControl) {
+		TraceWarning("Bogus IO request type %lu", params.Type);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
 
 	code = params.Parameters.DeviceIoControl.IoControlCode;
 	if (code != IOCTL_VIRT2PHYS_TRANSLATE) {
+		TraceWarning("Bogus IO control code %lx", code);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
@@ -200,6 +209,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveInputBuffer(
 			request, sizeof(*virt), (PVOID *)&virt, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving input buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -207,6 +217,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveOutputBuffer(
 		request, sizeof(*phys), (PVOID *)&phys, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving output buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -214,6 +225,9 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = virt2phys_translate(*virt, phys);
 	if (NT_SUCCESS(status))
 		WdfRequestSetInformation(request, sizeof(*phys));
+
+	TraceInfo("Translate %p to %llx: %!STATUS!",
+		virt, phys->QuadPart, status);
 	WdfRequestComplete(request, status);
 }
 
diff --git a/windows/virt2phys/virt2phys.vcxproj b/windows/virt2phys/virt2phys.vcxproj
index b462493..c9f884a 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -41,6 +41,7 @@
   <ItemGroup>
     <ClInclude Include="virt2phys.h" />
     <ClInclude Include="virt2phys_logic.h" />
+    <ClInclude Include="virt2phys_trace.h" />
   </ItemGroup>
   <ItemGroup>
     <Inf Include="virt2phys.inf" />
@@ -169,9 +170,9 @@
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
-      <WppEnabled>false</WppEnabled>
+      <WppEnabled>true</WppEnabled>
       <WppRecorderEnabled>true</WppRecorderEnabled>
-      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
+      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
     </ClCompile>
     <Link>
diff --git a/windows/virt2phys/virt2phys.vcxproj.filters b/windows/virt2phys/virt2phys.vcxproj.filters
index acd159f..6afff72 100644
--- a/windows/virt2phys/virt2phys.vcxproj.filters
+++ b/windows/virt2phys/virt2phys.vcxproj.filters
@@ -30,6 +30,9 @@
     <ClInclude Include="virt2phys_logic.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="virt2phys_trace.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virt2phys.c">
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
index 37b4dd4..e3ff293 100644
--- a/windows/virt2phys/virt2phys_logic.c
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -6,6 +6,8 @@
 #include <ntddk.h>
 
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys_logic.tmh"
 
 struct virt2phys_process {
 	HANDLE id;
@@ -38,6 +40,8 @@ virt2phys_block_create(PMDL mdl)
 static void
 virt2phys_block_free(struct virt2phys_block *block, BOOLEAN unmap)
 {
+	TraceInfo("VA = %p, unmap = %!bool!", block->mdl->StartVa, unmap);
+
 	if (unmap)
 		MmUnlockPages(block->mdl);
 
@@ -76,6 +80,8 @@ virt2phys_process_free(struct virt2phys_process *process, BOOLEAN unmap)
 	PSINGLE_LIST_ENTRY node;
 	struct virt2phys_block *block;
 
+	TraceInfo("ID = %p, unmap = %!bool!", process->id, unmap);
+
 	node = process->blocks.Next;
 	while (node != NULL) {
 		block = CONTAINING_RECORD(node, struct virt2phys_block, next);
@@ -208,6 +214,8 @@ virt2phys_add_block(struct virt2phys_process *process,
 	struct virt2phys_process *existing;
 	size_t size;
 
+	TraceInfo("ID = %p, VA = %p", process->id, block->mdl->StartVa);
+
 	existing = virt2phys_process_find(process->id);
 	*process_exists = existing != NULL;
 	if (existing == NULL) {
@@ -217,8 +225,11 @@ virt2phys_add_block(struct virt2phys_process *process,
 		 * because decrement is done without holding the lock.
 		 */
 		if (virt2phys_exceeeds(g_process_count + 1,
-				g_params.process_count_limit))
+				g_params.process_count_limit)) {
+			TraceWarning("Process count limit reached (%lu)",
+				g_params.process_count_limit);
 			return STATUS_QUOTA_EXCEEDED;
+		}
 
 		InsertHeadList(&g_processes, &process->next);
 		InterlockedIncrement(&g_process_count);
@@ -227,8 +238,11 @@ virt2phys_add_block(struct virt2phys_process *process,
 
 	size = MmGetMdlByteCount(block->mdl);
 	if (virt2phys_exceeeds(process->memory + size,
-			g_params.process_memory_limit))
+			g_params.process_memory_limit)) {
+		TraceWarning("Process %p memory limit reached (%llu bytes)",
+			process->id, g_params.process_memory_limit);
 		return STATUS_QUOTA_EXCEEDED;
+	}
 
 	PushEntryList(&process->blocks, &block->next);
 	process->memory += size;
@@ -277,8 +291,10 @@ virt2phys_check_memory(PMDL mdl)
 	size_t size;
 	NTSTATUS status;
 
-	if (!virt2phys_is_contiguous(mdl))
+	if (!virt2phys_is_contiguous(mdl)) {
+		TraceWarning("Locked region is not physycally contiguous");
 		return STATUS_UNSUCCESSFUL;
+	}
 
 	virt = MmGetMdlVirtualAddress(mdl);
 	size = MmGetMdlByteCount(mdl);
@@ -288,12 +304,19 @@ virt2phys_check_memory(PMDL mdl)
 	if (!NT_SUCCESS(status))
 		return status;
 
-	if (info.AllocationBase != virt || info.RegionSize != size)
+	if (info.AllocationBase != virt || info.RegionSize != size) {
+		TraceWarning("Race for the region: supplied %p (%llu bytes), locked %p (%llu bytes)",
+			virt, size, info.AllocationBase, info.RegionSize);
 		return STATUS_UNSUCCESSFUL;
-	if (info.State != MEM_COMMIT)
+	}
+	if (info.State != MEM_COMMIT) {
+		TraceWarning("Attempt to lock uncommitted memory");
 		return STATUS_UNSUCCESSFUL;
-	if (info.Type != MEM_PRIVATE)
+	}
+	if (info.Type != MEM_PRIVATE) {
+		TraceWarning("Attempt to lock shared memory");
 		return STATUS_UNSUCCESSFUL;
+	}
 	return status;
 }
 
diff --git a/windows/virt2phys/virt2phys_trace.h b/windows/virt2phys/virt2phys_trace.h
new file mode 100644
index 0000000..df863cb
--- /dev/null
+++ b/windows/virt2phys/virt2phys_trace.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Dmitry Kozlyuk
+ */
+
+/* Tracing GUID: C5C835BB-5CFB-4757-B1D4-9DD74662E212 */
+#define WPP_CONTROL_GUIDS \
+	WPP_DEFINE_CONTROL_GUID( \
+		VIRT2PHYS_TRACE_GUID, \
+		(C5C835BB, 5CFB, 4757, B1D4, 9DD74662E212), \
+		WPP_DEFINE_BIT(TRACE_GENERAL))
+
+#define WPP_FLAG_LEVEL_LOGGER(flag, level) \
+	WPP_LEVEL_LOGGER(flag)
+
+#define WPP_FLAG_LEVEL_ENABLED(flag, level) \
+	(WPP_LEVEL_ENABLED(flag) && \
+		WPP_CONTROL(WPP_BIT_ ## flag).Level >= level)
+
+#define WPP_LEVEL_FLAGS_LOGGER(lvl, flags) \
+	WPP_LEVEL_LOGGER(flags)
+
+#define WPP_LEVEL_FLAGS_ENABLED(lvl, flags) \
+	(WPP_LEVEL_ENABLED(flags) && \
+		WPP_CONTROL(WPP_BIT_ ## flags).Level >= lvl)
+
+/*
+ * WPP orders static parameters before dynamic parameters.
+ * To support trace functions defined below which sets FLAGS and LEVEL,
+ * a custom macro must be defined to reorder the arguments
+ * to what the .tpl configuration file expects.
+ */
+#define WPP_RECORDER_FLAGS_LEVEL_ARGS(flags, lvl) \
+	WPP_RECORDER_LEVEL_FLAGS_ARGS(lvl, flags)
+#define WPP_RECORDER_FLAGS_LEVEL_FILTER(flags, lvl) \
+	WPP_RECORDER_LEVEL_FLAGS_FILTER(lvl, flags)
+
+/*
+begin_wpp config
+
+USEPREFIX(TraceError, "[%!FUNC!] ");
+FUNC TraceError{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_ERROR}(MSG, ...);
+
+USEPREFIX(TraceWarning, "[%!FUNC!] ");
+FUNC TraceWarning{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_WARNING}(MSG, ...);
+
+USEPREFIX(TraceInfo, "[%!FUNC!] ");
+FUNC TraceInfo{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_INFORMATION}(MSG, ...);
+
+end_wpp
+*/
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing Dmitry Kozlyuk
@ 2021-09-30 22:07     ` Menon, Ranjit
  2021-09-30 22:13       ` Menon, Ranjit
  2021-10-01  7:10       ` Dmitry Kozlyuk
  0 siblings, 2 replies; 21+ messages in thread
From: Menon, Ranjit @ 2021-09-30 22:07 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly

Hi Dmitry,

On 5/26/2021 2:01 PM, Dmitry Kozlyuk wrote:
> WPP tracing [1] allows kernel drivers to print logs that can be viewed
> without attaching a debugger to the running system. Traces are colelcted
> only when enabled. Instrument virt2phys with traces:
> * ERROR:   failures that prevent the driver from working.
> * WARNING: incorrect calls to the driver.
> * INFO:    starting or completing operations with memory.
>
> [1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/wpp-software-tracing
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> <snip!>
diff --git a/windows/virt2phys/virt2phys.vcxproj 
b/windows/virt2phys/virt2phys.vcxproj
> index b462493..c9f884a 100644
> --- a/windows/virt2phys/virt2phys.vcxproj
> +++ b/windows/virt2phys/virt2phys.vcxproj
> @@ -41,6 +41,7 @@
>     <ItemGroup>
>       <ClInclude Include="virt2phys.h" />
>       <ClInclude Include="virt2phys_logic.h" />
> +    <ClInclude Include="virt2phys_trace.h" />
>     </ItemGroup>
>     <ItemGroup>
>       <Inf Include="virt2phys.inf" />
> @@ -169,9 +170,9 @@
>     </ItemDefinitionGroup>
>     <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
>       <ClCompile>
> -      <WppEnabled>false</WppEnabled>
> +      <WppEnabled>true</WppEnabled>
>         <WppRecorderEnabled>true</WppRecorderEnabled>
> -      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
> +      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>

This change is also required for the 'Release|x64' configuration, 
otherwise 'Release' builds fail.

<snip!>

Also, it appears the newer version of the compiler (combined with the 
new version of WDK/SDK), requires that the driver signing process 
mandate a File Digest Algorithm (using the /fd option). It is a warning 
today, but they claim it could become a requirement in the future.

To fix this, we can include the following in the project file:

<DriverSign>
   <FileDigestAlgorithm>SHA256</FileDigestAlgorithm>
</DriverSign>

Or set the above, using project 'Properties'->Driver Signing->File 
Digest Algorithm = 256.

(This will need to be fixed in the netuio driver project also)

thanks,

ranjit m.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing
  2021-09-30 22:07     ` Menon, Ranjit
@ 2021-09-30 22:13       ` Menon, Ranjit
  2021-10-01  7:10       ` Dmitry Kozlyuk
  1 sibling, 0 replies; 21+ messages in thread
From: Menon, Ranjit @ 2021-09-30 22:13 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly

Typo below:

On 9/30/2021 3:07 PM, Menon, Ranjit wrote:
> Hi Dmitry,
>
> On 5/26/2021 2:01 PM, Dmitry Kozlyuk wrote:
>> WPP tracing [1] allows kernel drivers to print logs that can be viewed
>> without attaching a debugger to the running system. Traces are colelcted
>> only when enabled. Instrument virt2phys with traces:
>> * ERROR:   failures that prevent the driver from working.
>> * WARNING: incorrect calls to the driver.
>> * INFO:    starting or completing operations with memory.
>>
>> [1]: 
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/wpp-software-tracing
>>
>> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>> ---
>> <snip!>
> diff --git a/windows/virt2phys/virt2phys.vcxproj 
> b/windows/virt2phys/virt2phys.vcxproj
>> index b462493..c9f884a 100644
>> --- a/windows/virt2phys/virt2phys.vcxproj
>> +++ b/windows/virt2phys/virt2phys.vcxproj
>> @@ -41,6 +41,7 @@
>>     <ItemGroup>
>>       <ClInclude Include="virt2phys.h" />
>>       <ClInclude Include="virt2phys_logic.h" />
>> +    <ClInclude Include="virt2phys_trace.h" />
>>     </ItemGroup>
>>     <ItemGroup>
>>       <Inf Include="virt2phys.inf" />
>> @@ -169,9 +170,9 @@
>>     </ItemDefinitionGroup>
>>     <ItemDefinitionGroup 
>> Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
>>       <ClCompile>
>> -      <WppEnabled>false</WppEnabled>
>> +      <WppEnabled>true</WppEnabled>
>> <WppRecorderEnabled>true</WppRecorderEnabled>
>> -      <WppScanConfigurationData 
>> Condition="'%(ClCompile.ScanConfigurationData)' == 
>> ''">trace.h</WppScanConfigurationData>
>> +      <WppScanConfigurationData 
>> Condition="'%(ClCompile.ScanConfigurationData)' == 
>> ''">virt2phys_trace.h</WppScanConfigurationData>
>
> This change is also required for the 'Release|x64' configuration, 
> otherwise 'Release' builds fail.
>
> <snip!>
>
> Also, it appears the newer version of the compiler (combined with the 
> new version of WDK/SDK), requires that the driver signing process 
> mandate a File Digest Algorithm (using the /fd option). It is a 
> warning today, but they claim it could become a requirement in the 
> future.
>
> To fix this, we can include the following in the project file:
>
> <DriverSign>
>   <FileDigestAlgorithm>SHA256</FileDigestAlgorithm>
> </DriverSign>
>
> Or set the above, using project 'Properties'->Driver Signing->File 
> Digest Algorithm = 256.

Should be:

"Or set the above, using project 'Properties'->Driver Signing->File 
Digest Algorithm = SHA256"

>
> (This will need to be fixed in the netuio driver project also)
>
> thanks,
>
> ranjit m.
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing
  2021-09-30 22:07     ` Menon, Ranjit
  2021-09-30 22:13       ` Menon, Ranjit
@ 2021-10-01  7:10       ` Dmitry Kozlyuk
  1 sibling, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-10-01  7:10 UTC (permalink / raw)
  To: Menon, Ranjit
  Cc: dev, Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly

2021-09-30 15:07 (UTC-0700), Menon, Ranjit:
> [...]
> >     <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
> >       <ClCompile>
> > -      <WppEnabled>false</WppEnabled>
> > +      <WppEnabled>true</WppEnabled>
> >         <WppRecorderEnabled>true</WppRecorderEnabled>
> > -      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
> > +      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>  
> 
> This change is also required for the 'Release|x64' configuration, 
> otherwise 'Release' builds fail.

Thanks Ranjit, will fix in v3.

> <snip!>
> 
> Also, it appears the newer version of the compiler (combined with the 
> new version of WDK/SDK), requires that the driver signing process 
> mandate a File Digest Algorithm (using the /fd option). It is a warning 
> today, but they claim it could become a requirement in the future.

Yes, it started with WDK 22000.1. On my system it's an error, not a warning.
I think I'll create a separate series with trivial fixes like this, so that
it can be merged quickly and fix the build with newer WDK.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
                     ` (3 preceding siblings ...)
  2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing Dmitry Kozlyuk
@ 2021-06-23  7:13   ` Thomas Monjalon
  2021-09-30 20:24     ` Thomas Monjalon
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
  5 siblings, 1 reply; 21+ messages in thread
From: Thomas Monjalon @ 2021-06-23  7:13 UTC (permalink / raw)
  To: Dmitry Malloy, Tyler Retzlaff
  Cc: dev, Narcisa Ana Maria Vasile, Pallavi Kadam, Nick Connolly,
	Dmitry Kozlyuk, Harini Ramakrishnan

26/05/2021 23:01, Dmitry Kozlyuk:
> v2:
>     * Following ofline review by DmitryM:
>       - Add comment explaining tracking approach for validation team.
>       - Replace deprecated allocation API calls.
>       - Check properties of locked memory (see docs in patch 3/4).
>       - Add configurable limits for tracked processes and memory.
>     * Add end-user documentation.
>     * Drop patch for Inf2Cat settings UseLocalTime=true:
>       the issue it resolves originated from development VM.
>     * Add PnpLockdown=1 patch.
> 
> Dmitry Kozlyuk (4):
>   windows/virt2phys: add PnpLockdown directive
>   windows/virt2phys: do not expose pageable physical addresses
>   windows/virt2phys: add limits against resource exhaustion
>   windows/virt2phys: add tracing

Waiting for reviews, especially from Microsoft experts.




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue
  2021-06-23  7:13   ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Thomas Monjalon
@ 2021-09-30 20:24     ` Thomas Monjalon
  2021-09-30 20:39       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Monjalon @ 2021-09-30 20:24 UTC (permalink / raw)
  To: Dmitry Malloy, Tyler Retzlaff, Narcisa Ana Maria Vasile,
	Harini Ramakrishnan
  Cc: dev, Pallavi Kadam, Nick Connolly, Dmitry Kozlyuk

23/06/2021 09:13, Thomas Monjalon:
> 26/05/2021 23:01, Dmitry Kozlyuk:
> > v2:
> >     * Following ofline review by DmitryM:
> >       - Add comment explaining tracking approach for validation team.
> >       - Replace deprecated allocation API calls.
> >       - Check properties of locked memory (see docs in patch 3/4).
> >       - Add configurable limits for tracked processes and memory.
> >     * Add end-user documentation.
> >     * Drop patch for Inf2Cat settings UseLocalTime=true:
> >       the issue it resolves originated from development VM.
> >     * Add PnpLockdown=1 patch.
> > 
> > Dmitry Kozlyuk (4):
> >   windows/virt2phys: add PnpLockdown directive
> >   windows/virt2phys: do not expose pageable physical addresses
> >   windows/virt2phys: add limits against resource exhaustion
> >   windows/virt2phys: add tracing
> 
> Waiting for reviews, especially from Microsoft experts.

Why is it taking so long?



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue
  2021-09-30 20:24     ` Thomas Monjalon
@ 2021-09-30 20:39       ` Dmitry Kozlyuk
  0 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-09-30 20:39 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Dmitry Malloy, Tyler Retzlaff, Narcisa Ana Maria Vasile,
	Harini Ramakrishnan, dev, Pallavi Kadam, Nick Connolly

2021-09-30 22:24 (UTC+0200), Thomas Monjalon:
> 23/06/2021 09:13, Thomas Monjalon:
> > 26/05/2021 23:01, Dmitry Kozlyuk:  
> > > v2:
> > >     * Following ofline review by DmitryM:
> > >       - Add comment explaining tracking approach for validation team.
> > >       - Replace deprecated allocation API calls.
> > >       - Check properties of locked memory (see docs in patch 3/4).
> > >       - Add configurable limits for tracked processes and memory.
> > >     * Add end-user documentation.
> > >     * Drop patch for Inf2Cat settings UseLocalTime=true:
> > >       the issue it resolves originated from development VM.
> > >     * Add PnpLockdown=1 patch.
> > > 
> > > Dmitry Kozlyuk (4):
> > >   windows/virt2phys: add PnpLockdown directive
> > >   windows/virt2phys: do not expose pageable physical addresses
> > >   windows/virt2phys: add limits against resource exhaustion
> > >   windows/virt2phys: add tracing  
> > 
> > Waiting for reviews, especially from Microsoft experts.  
> 
> Why is it taking so long?

DmitryM, Microsoft expert who suggested this improvement, has been unavailable
until recently. It is expected that he will review this series shortly now.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v3 0/3] windows/virt2phys: fix paging issue
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
                     ` (4 preceding siblings ...)
  2021-06-23  7:13   ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Thomas Monjalon
@ 2021-10-12  0:42   ` Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 1/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
                       ` (3 more replies)
  5 siblings, 4 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-10-12  0:42 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, Ranjit Menon

Physical addresses exposed by virt2phys driver could become pageable.
This presents stability and security issues that prevent Microsoft
from signing virt2phys, because a signed driver would be trusted
by all end-user machines.

Ensure that memory for which physical addresses are exposed by
virt2phys is non-pageable at least for the lifetime of the process.
As virt2phys code grows, make its development and debugging easier.

There are other known issues that come from using PA and accessing DMA
from userspace. They are not related to virt2phys par se. It is planned
to address them later by enabling the use of IOMMU for DPDK on Windows.

Depends-on: series-19342 ("windows: independent fixes")

v3:
    * Fix Release build (Ranjit).
    * Drop PnpLockdown=1 patch as it is now in dependency series.
v2:
    * Following ofline review by DmitryM:
      - Add comment explaining tracking approach for validation team.
      - Replace deprecated allocation API calls.
      - Check properties of locked memory (see docs in patch 3/4).
      - Add configurable limits for tracked processes and memory.
    * Add end-user documentation.
    * Drop patch for Inf2Cat settings UseLocalTime=true:
      the issue it resolves originated from development VM.
    * Add PnpLockdown=1 patch.


Dmitry Kozlyuk (3):
  windows/virt2phys: do not expose pageable physical addresses
  windows/virt2phys: add limits against resource exhaustion
  windows/virt2phys: add tracing

 windows/virt2phys/README.md                 |  38 ++
 windows/virt2phys/virt2phys.c               | 173 ++++++--
 windows/virt2phys/virt2phys.vcxproj         |  11 +-
 windows/virt2phys/virt2phys.vcxproj.filters |  11 +-
 windows/virt2phys/virt2phys_logic.c         | 415 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  39 ++
 windows/virt2phys/virt2phys_trace.h         |  50 +++
 7 files changed, 703 insertions(+), 34 deletions(-)
 create mode 100644 windows/virt2phys/README.md
 create mode 100644 windows/virt2phys/virt2phys_logic.c
 create mode 100644 windows/virt2phys/virt2phys_logic.h
 create mode 100644 windows/virt2phys/virt2phys_trace.h

-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v3 1/3] windows/virt2phys: do not expose pageable physical addresses
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
@ 2021-10-12  0:42     ` Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 2/3] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-10-12  0:42 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Kozlyuk, Dmitry Malloy, Ranjit Menon,
	Narcisa Ana Maria Vasile, Pallavi Kadam

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>
Acked-by: Ranjit Menon <ranjit.menon@intel.com>
---
 windows/virt2phys/virt2phys.c               |  92 +++--
 windows/virt2phys/virt2phys.vcxproj         |   2 +
 windows/virt2phys/virt2phys.vcxproj.filters |   8 +-
 windows/virt2phys/virt2phys_logic.c         | 368 ++++++++++++++++++++
 windows/virt2phys/virt2phys_logic.h         |  32 ++
 5 files changed, 471 insertions(+), 31 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 6794452..656b1f2 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -12,9 +12,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..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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v3 2/3] windows/virt2phys: add limits against resource exhaustion
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 1/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
@ 2021-10-12  0:42     ` Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
  2022-01-11 13:56     ` [dpdk-dev] [kmods PATCH v3 0/3] windows/virt2phys: fix paging issue Thomas Monjalon
  3 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-10-12  0:42 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Kozlyuk, Ranjit Menon, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam

Tracked processes are enumerated in process creation/termination
callback, making it O(N = number of tracked processes). A malicious user
could cause system-wide slowdown of process termination by making just one
call to virt2phys from many processes. Limit the number of tracked
processes by `ProcessCountLimit` items.

Any process with access to virt2phys could exhaust RAM for all the
system by locking it. It can also exhaust RAM needed for other processes
with access to virt2phys. Limit amount of memory locked by each tracked
process with `ProcessMemoryLimitMB`.

All limits are read from registry at driver load.
Each limit can be turned off by setting it to 0.
Document limits and their non-zero defaults for administrators.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Ranjit Menon <ranjit.menon@intel.com>
---
 windows/virt2phys/README.md         | 38 +++++++++++++++
 windows/virt2phys/virt2phys.c       | 73 ++++++++++++++++++++++++++++-
 windows/virt2phys/virt2phys_logic.c | 46 +++++++++++++-----
 windows/virt2phys/virt2phys_logic.h |  9 +++-
 4 files changed, 152 insertions(+), 14 deletions(-)
 create mode 100644 windows/virt2phys/README.md

diff --git a/windows/virt2phys/README.md b/windows/virt2phys/README.md
new file mode 100644
index 0000000..04012ff
--- /dev/null
+++ b/windows/virt2phys/README.md
@@ -0,0 +1,38 @@
+Virtual to Physical Address Translator
+======================================
+
+Purpose and Operation
+---------------------
+
+``virt2phys`` driver allows user-mode processes to obtain physical address
+of a given virtual address in their address space.
+Virtual addresses must belong to regions from process private working set.
+These regions must be physically contiguous.
+The driver ensures that memory regions with translated addresses
+are not swapped out as long as the process has access to this memory.
+
+It is not safe to administratively unload the driver
+while there are processes that have used virt2phys to translate addresses.
+Doing so will permanently leak RAM occupied by all memory regions
+that contain translated addresses.
+Terminate all such processes before unloading the driver.
+
+Configuration
+-------------
+
+``virt2phys`` is configured at loading time via registry key
+``HKLM\SYSTEM\ControlSet001\Services\virt2phys\Parameters``.
+
+* ``ProcessCountLimit`` (default 16)
+
+  Maximum number of processes that can have access to memory regions
+  with translated addresses. When this limit is reached, no more processes
+  can translate addresses using ``virt2phys``. Large number of tracked
+  processes may slow down system operation. Set limit to 0 to disable it.
+
+* ``ProcessMemoryLimitMB`` (default 16384, i.e. 16 GB)
+
+  Maximum amount of memory in all regions that contain translated addresses,
+  total per process. When this limit is reached, the process can not translate
+  addresses from new regions. Large values can cause RAM exhaustion.
+  Set limit to 0 to disable it.
\ No newline at end of file
diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index bf0c300..44204c9 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -14,15 +14,22 @@ 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 NTSTATUS virt2phys_load_params(
+	WDFDRIVER driver, struct virt2phys_params *params);
 static VOID virt2phys_on_process_event(
 	HANDLE parent_id, HANDLE process_id, BOOLEAN create);
 
+static const ULONG PROCESS_COUNT_LIMIT_DEF = 1 << 4;
+static const ULONG PROCESS_MEMORY_LIMIT_DEF = 16 * (1 << 10); /* MB */
+
 _Use_decl_annotations_
 NTSTATUS
 DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 {
 	WDF_DRIVER_CONFIG config;
 	WDF_OBJECT_ATTRIBUTES attributes;
+	WDFDRIVER driver;
+	struct virt2phys_params params;
 	NTSTATUS status;
 
 	PAGED_CODE();
@@ -32,11 +39,15 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	config.EvtDriverUnload = virt2phys_driver_unload;
 	status = WdfDriverCreate(
 		driver_object, registry_path,
-		&attributes, &config, WDF_NO_HANDLE);
+		&attributes, &config, &driver);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_load_params(driver, &params);
 	if (!NT_SUCCESS(status))
 		return status;
 
-	status = virt2phys_init();
+	status = virt2phys_init(&params);
 	if (!NT_SUCCESS(status))
 		return status;
 
@@ -58,6 +69,64 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	return status;
 }
 
+static NTSTATUS
+virt2phys_read_param(WDFKEY key, PCUNICODE_STRING name, ULONG *value,
+	ULONG def)
+{
+	NTSTATUS status;
+
+	status = WdfRegistryQueryULong(key, name, value);
+	if (status == STATUS_OBJECT_NAME_NOT_FOUND) {
+		*value = def;
+		status = STATUS_SUCCESS;
+	}
+	return status;
+}
+
+static NTSTATUS
+virt2phys_read_mb(WDFKEY key, PCUNICODE_STRING name, ULONG64 *bytes,
+	ULONG def_mb)
+{
+	ULONG mb;
+	NTSTATUS status;
+
+	status = virt2phys_read_param(key, name, &mb, def_mb);
+	if (NT_SUCCESS(status))
+		*bytes = (ULONG64)mb * (1ULL << 20);
+	return status;
+}
+
+static NTSTATUS
+virt2phys_load_params(WDFDRIVER driver, struct virt2phys_params *params)
+{
+	static DECLARE_CONST_UNICODE_STRING(
+		process_count_limit, L"ProcessCountLimit");
+	static DECLARE_CONST_UNICODE_STRING(
+		process_memory_limit, L"ProcessMemoryLimitMB");
+
+	WDFKEY key;
+	NTSTATUS status;
+
+	status = WdfDriverOpenParametersRegistryKey(
+		driver, KEY_READ, WDF_NO_OBJECT_ATTRIBUTES, &key);
+	if (!NT_SUCCESS(status))
+		return status;
+
+	status = virt2phys_read_param(key, &process_count_limit,
+		&params->process_count_limit, PROCESS_COUNT_LIMIT_DEF);
+	if (!NT_SUCCESS(status))
+		goto cleanup;
+
+	status = virt2phys_read_mb(key, &process_memory_limit,
+		&params->process_memory_limit, PROCESS_MEMORY_LIMIT_DEF);
+	if (!NT_SUCCESS(status))
+		goto cleanup;
+
+cleanup:
+	WdfRegistryClose(key);
+	return status;
+}
+
 _Use_decl_annotations_
 VOID
 virt2phys_driver_unload(WDFDRIVER driver)
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
index a27802c..37b4dd4 100644
--- a/windows/virt2phys/virt2phys_logic.c
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -11,6 +11,7 @@ struct virt2phys_process {
 	HANDLE id;
 	LIST_ENTRY next;
 	SINGLE_LIST_ENTRY blocks;
+	ULONG64 memory;
 };
 
 struct virt2phys_block {
@@ -18,7 +19,9 @@ struct virt2phys_block {
 	SINGLE_LIST_ENTRY next;
 };
 
+static struct virt2phys_params g_params;
 static LIST_ENTRY g_processes;
+static LONG g_process_count;
 static PKSPIN_LOCK g_lock;
 
 struct virt2phys_block *
@@ -112,7 +115,7 @@ virt2phys_process_find_block(struct virt2phys_process *process, PVOID virt)
 }
 
 NTSTATUS
-virt2phys_init(void)
+virt2phys_init(const struct virt2phys_params *params)
 {
 	g_lock = ExAllocatePoolZero(NonPagedPool, sizeof(*g_lock), 'gp2v');
 	if (g_lock == NULL)
@@ -120,6 +123,7 @@ virt2phys_init(void)
 
 	InitializeListHead(&g_processes);
 
+	g_params = *params;
 	return STATUS_SUCCESS;
 }
 
@@ -165,8 +169,10 @@ virt2phys_process_cleanup(HANDLE process_id)
 	process = virt2phys_process_detach(process_id);
 	KeReleaseSpinLock(g_lock, irql);
 
-	if (process != NULL)
+	if (process != NULL) {
 		virt2phys_process_free(process, TRUE);
+		InterlockedDecrement(&g_process_count);
+	}
 }
 
 static struct virt2phys_block *
@@ -195,21 +201,38 @@ virt2phys_exceeeds(LONG64 count, ULONG64 limit)
 	return limit > 0 && count > (LONG64)limit;
 }
 
-static BOOLEAN
+static NTSTATUS
 virt2phys_add_block(struct virt2phys_process *process,
-	struct virt2phys_block *block)
+	struct virt2phys_block *block, BOOLEAN *process_exists)
 {
 	struct virt2phys_process *existing;
+	size_t size;
 
 	existing = virt2phys_process_find(process->id);
-	if (existing == NULL)
+	*process_exists = existing != NULL;
+	if (existing == NULL) {
+		/*
+		 * This check is done with the lock held so that's no race.
+		 * Increment below must be atomic however,
+		 * because decrement is done without holding the lock.
+		 */
+		if (virt2phys_exceeeds(g_process_count + 1,
+				g_params.process_count_limit))
+			return STATUS_QUOTA_EXCEEDED;
+
 		InsertHeadList(&g_processes, &process->next);
-	else
+		InterlockedIncrement(&g_process_count);
+	} else
 		process = existing;
 
-	PushEntryList(&process->blocks, &block->next);
+	size = MmGetMdlByteCount(block->mdl);
+	if (virt2phys_exceeeds(process->memory + size,
+			g_params.process_memory_limit))
+		return STATUS_QUOTA_EXCEEDED;
 
-	return existing != NULL;
+	PushEntryList(&process->blocks, &block->next);
+	process->memory += size;
+	return STATUS_SUCCESS;
 }
 
 static NTSTATUS
@@ -356,13 +379,14 @@ virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys)
 	}
 
 	KeAcquireSpinLock(g_lock, &irql);
-	tracked = virt2phys_add_block(process, block);
+	status = virt2phys_add_block(process, block, &tracked);
 	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;
+	if (NT_SUCCESS(status))
+		*phys = virt2phys_block_translate(block, virt);
+	return status;
 }
diff --git a/windows/virt2phys/virt2phys_logic.h b/windows/virt2phys/virt2phys_logic.h
index 1582206..c8255f9 100644
--- a/windows/virt2phys/virt2phys_logic.h
+++ b/windows/virt2phys/virt2phys_logic.h
@@ -5,10 +5,17 @@
 #ifndef VIRT2PHYS_LOGIC_H
 #define VIRT2PHYS_LOGIC_H
 
+struct virt2phys_params {
+	/** Maximum number of tracked processes (0 = unlimited). */
+	ULONG process_count_limit;
+	/** Maximum amount of memory locked by a process (0 = unlimited). */
+	ULONG64 process_memory_limit;
+};
+
 /**
  * Initialize internal data structures.
  */
-NTSTATUS virt2phys_init(void);
+NTSTATUS virt2phys_init(const struct virt2phys_params *params);
 
 /**
  * Free memory allocated for internal data structures.
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [kmods PATCH v3 3/3] windows/virt2phys: add tracing
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 1/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 2/3] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
@ 2021-10-12  0:42     ` Dmitry Kozlyuk
  2022-01-11 13:56     ` [dpdk-dev] [kmods PATCH v3 0/3] windows/virt2phys: fix paging issue Thomas Monjalon
  3 siblings, 0 replies; 21+ messages in thread
From: Dmitry Kozlyuk @ 2021-10-12  0:42 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Kozlyuk, Ranjit Menon, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam

WPP tracing [1] allows kernel drivers to print logs that can be viewed
without attaching a debugger to the running system. Traces are colelcted
only when enabled. Instrument virt2phys with traces:
* ERROR:   failures that prevent the driver from working.
* WARNING: incorrect calls to the driver.
* INFO:    starting or completing operations with memory.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/wpp-software-tracing

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Ranjit Menon <ranjit.menon@intel.com>
---
 windows/virt2phys/virt2phys.c               | 18 +++++++-
 windows/virt2phys/virt2phys.vcxproj         |  9 ++--
 windows/virt2phys/virt2phys.vcxproj.filters |  3 ++
 windows/virt2phys/virt2phys_logic.c         | 35 ++++++++++++---
 windows/virt2phys/virt2phys_trace.h         | 50 +++++++++++++++++++++
 5 files changed, 103 insertions(+), 12 deletions(-)
 create mode 100644 windows/virt2phys/virt2phys_trace.h

diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c
index 44204c9..f4d5298 100644
--- a/windows/virt2phys/virt2phys.c
+++ b/windows/virt2phys/virt2phys.c
@@ -8,6 +8,8 @@
 
 #include "virt2phys.h"
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys.tmh"
 
 DRIVER_INITIALIZE DriverEntry;
 EVT_WDF_DRIVER_UNLOAD virt2phys_driver_unload;
@@ -66,6 +68,8 @@ DriverEntry(PDRIVER_OBJECT driver_object, PUNICODE_STRING registry_path)
 	if (!NT_SUCCESS(status))
 		return status;
 
+	WPP_INIT_TRACING(driver_object, registry_path);
+
 	return status;
 }
 
@@ -131,11 +135,11 @@ _Use_decl_annotations_
 VOID
 virt2phys_driver_unload(WDFDRIVER driver)
 {
-	UNREFERENCED_PARAMETER(driver);
-
 	PsSetCreateProcessNotifyRoutine(virt2phys_on_process_event, TRUE);
 
 	virt2phys_cleanup();
+
+	WPP_CLEANUP(WdfDriverWdmGetDriverObject(driver));
 }
 
 _Use_decl_annotations_
@@ -157,12 +161,15 @@ virt2phys_driver_EvtDeviceAdd(WDFDRIVER driver, PWDFDEVICE_INIT init)
 
 	status = WdfDeviceCreate(&init, &attributes, &device);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDriverCreate() = %!STATUS!", status);
 		return status;
 	}
 
 	status = WdfDeviceCreateDeviceInterface(
 		device, &GUID_DEVINTERFACE_VIRT2PHYS, NULL);
 	if (!NT_SUCCESS(status)) {
+		TraceError("WdfDeviceCreateDeviceInterface() = %!STATUS!",
+			status);
 		return status;
 	}
 
@@ -187,12 +194,14 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	WdfRequestGetParameters(request, &params);
 
 	if (params.Type != WdfRequestTypeDeviceControl) {
+		TraceWarning("Bogus IO request type %lu", params.Type);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
 
 	code = params.Parameters.DeviceIoControl.IoControlCode;
 	if (code != IOCTL_VIRT2PHYS_TRANSLATE) {
+		TraceWarning("Bogus IO control code %lx", code);
 		WdfRequestComplete(request, STATUS_NOT_SUPPORTED);
 		return;
 	}
@@ -200,6 +209,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveInputBuffer(
 			request, sizeof(*virt), (PVOID *)&virt, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving input buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -207,6 +217,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = WdfRequestRetrieveOutputBuffer(
 		request, sizeof(*phys), (PVOID *)&phys, &size);
 	if (!NT_SUCCESS(status)) {
+		TraceWarning("Retrieving output buffer: %!STATUS!", status);
 		WdfRequestComplete(request, status);
 		return;
 	}
@@ -214,6 +225,9 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request)
 	status = virt2phys_translate(*virt, phys);
 	if (NT_SUCCESS(status))
 		WdfRequestSetInformation(request, sizeof(*phys));
+
+	TraceInfo("Translate %p to %llx: %!STATUS!",
+		virt, phys->QuadPart, status);
 	WdfRequestComplete(request, status);
 }
 
diff --git a/windows/virt2phys/virt2phys.vcxproj b/windows/virt2phys/virt2phys.vcxproj
index 656b1f2..fd168b1 100644
--- a/windows/virt2phys/virt2phys.vcxproj
+++ b/windows/virt2phys/virt2phys.vcxproj
@@ -17,6 +17,7 @@
   <ItemGroup>
     <ClInclude Include="virt2phys.h" />
     <ClInclude Include="virt2phys_logic.h" />
+    <ClInclude Include="virt2phys_trace.h" />
   </ItemGroup>
   <ItemGroup>
     <Inf Include="virt2phys.inf" />
@@ -63,9 +64,9 @@
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <ClCompile>
-      <WppEnabled>false</WppEnabled>
+      <WppEnabled>true</WppEnabled>
       <WppRecorderEnabled>true</WppRecorderEnabled>
-      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
+      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
     </ClCompile>
     <Link>
@@ -80,9 +81,9 @@
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <ClCompile>
-      <WppEnabled>false</WppEnabled>
+      <WppEnabled>true</WppEnabled>
       <WppRecorderEnabled>true</WppRecorderEnabled>
-      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
+      <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">virt2phys_trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
     </ClCompile>
     <DriverSign>
diff --git a/windows/virt2phys/virt2phys.vcxproj.filters b/windows/virt2phys/virt2phys.vcxproj.filters
index 6b65d71..b2f6776 100644
--- a/windows/virt2phys/virt2phys.vcxproj.filters
+++ b/windows/virt2phys/virt2phys.vcxproj.filters
@@ -30,6 +30,9 @@
     <ClInclude Include="virt2phys_logic.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="virt2phys_trace.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virt2phys.c">
diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c
index 37b4dd4..e3ff293 100644
--- a/windows/virt2phys/virt2phys_logic.c
+++ b/windows/virt2phys/virt2phys_logic.c
@@ -6,6 +6,8 @@
 #include <ntddk.h>
 
 #include "virt2phys_logic.h"
+#include "virt2phys_trace.h"
+#include "virt2phys_logic.tmh"
 
 struct virt2phys_process {
 	HANDLE id;
@@ -38,6 +40,8 @@ virt2phys_block_create(PMDL mdl)
 static void
 virt2phys_block_free(struct virt2phys_block *block, BOOLEAN unmap)
 {
+	TraceInfo("VA = %p, unmap = %!bool!", block->mdl->StartVa, unmap);
+
 	if (unmap)
 		MmUnlockPages(block->mdl);
 
@@ -76,6 +80,8 @@ virt2phys_process_free(struct virt2phys_process *process, BOOLEAN unmap)
 	PSINGLE_LIST_ENTRY node;
 	struct virt2phys_block *block;
 
+	TraceInfo("ID = %p, unmap = %!bool!", process->id, unmap);
+
 	node = process->blocks.Next;
 	while (node != NULL) {
 		block = CONTAINING_RECORD(node, struct virt2phys_block, next);
@@ -208,6 +214,8 @@ virt2phys_add_block(struct virt2phys_process *process,
 	struct virt2phys_process *existing;
 	size_t size;
 
+	TraceInfo("ID = %p, VA = %p", process->id, block->mdl->StartVa);
+
 	existing = virt2phys_process_find(process->id);
 	*process_exists = existing != NULL;
 	if (existing == NULL) {
@@ -217,8 +225,11 @@ virt2phys_add_block(struct virt2phys_process *process,
 		 * because decrement is done without holding the lock.
 		 */
 		if (virt2phys_exceeeds(g_process_count + 1,
-				g_params.process_count_limit))
+				g_params.process_count_limit)) {
+			TraceWarning("Process count limit reached (%lu)",
+				g_params.process_count_limit);
 			return STATUS_QUOTA_EXCEEDED;
+		}
 
 		InsertHeadList(&g_processes, &process->next);
 		InterlockedIncrement(&g_process_count);
@@ -227,8 +238,11 @@ virt2phys_add_block(struct virt2phys_process *process,
 
 	size = MmGetMdlByteCount(block->mdl);
 	if (virt2phys_exceeeds(process->memory + size,
-			g_params.process_memory_limit))
+			g_params.process_memory_limit)) {
+		TraceWarning("Process %p memory limit reached (%llu bytes)",
+			process->id, g_params.process_memory_limit);
 		return STATUS_QUOTA_EXCEEDED;
+	}
 
 	PushEntryList(&process->blocks, &block->next);
 	process->memory += size;
@@ -277,8 +291,10 @@ virt2phys_check_memory(PMDL mdl)
 	size_t size;
 	NTSTATUS status;
 
-	if (!virt2phys_is_contiguous(mdl))
+	if (!virt2phys_is_contiguous(mdl)) {
+		TraceWarning("Locked region is not physycally contiguous");
 		return STATUS_UNSUCCESSFUL;
+	}
 
 	virt = MmGetMdlVirtualAddress(mdl);
 	size = MmGetMdlByteCount(mdl);
@@ -288,12 +304,19 @@ virt2phys_check_memory(PMDL mdl)
 	if (!NT_SUCCESS(status))
 		return status;
 
-	if (info.AllocationBase != virt || info.RegionSize != size)
+	if (info.AllocationBase != virt || info.RegionSize != size) {
+		TraceWarning("Race for the region: supplied %p (%llu bytes), locked %p (%llu bytes)",
+			virt, size, info.AllocationBase, info.RegionSize);
 		return STATUS_UNSUCCESSFUL;
-	if (info.State != MEM_COMMIT)
+	}
+	if (info.State != MEM_COMMIT) {
+		TraceWarning("Attempt to lock uncommitted memory");
 		return STATUS_UNSUCCESSFUL;
-	if (info.Type != MEM_PRIVATE)
+	}
+	if (info.Type != MEM_PRIVATE) {
+		TraceWarning("Attempt to lock shared memory");
 		return STATUS_UNSUCCESSFUL;
+	}
 	return status;
 }
 
diff --git a/windows/virt2phys/virt2phys_trace.h b/windows/virt2phys/virt2phys_trace.h
new file mode 100644
index 0000000..df863cb
--- /dev/null
+++ b/windows/virt2phys/virt2phys_trace.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 Dmitry Kozlyuk
+ */
+
+/* Tracing GUID: C5C835BB-5CFB-4757-B1D4-9DD74662E212 */
+#define WPP_CONTROL_GUIDS \
+	WPP_DEFINE_CONTROL_GUID( \
+		VIRT2PHYS_TRACE_GUID, \
+		(C5C835BB, 5CFB, 4757, B1D4, 9DD74662E212), \
+		WPP_DEFINE_BIT(TRACE_GENERAL))
+
+#define WPP_FLAG_LEVEL_LOGGER(flag, level) \
+	WPP_LEVEL_LOGGER(flag)
+
+#define WPP_FLAG_LEVEL_ENABLED(flag, level) \
+	(WPP_LEVEL_ENABLED(flag) && \
+		WPP_CONTROL(WPP_BIT_ ## flag).Level >= level)
+
+#define WPP_LEVEL_FLAGS_LOGGER(lvl, flags) \
+	WPP_LEVEL_LOGGER(flags)
+
+#define WPP_LEVEL_FLAGS_ENABLED(lvl, flags) \
+	(WPP_LEVEL_ENABLED(flags) && \
+		WPP_CONTROL(WPP_BIT_ ## flags).Level >= lvl)
+
+/*
+ * WPP orders static parameters before dynamic parameters.
+ * To support trace functions defined below which sets FLAGS and LEVEL,
+ * a custom macro must be defined to reorder the arguments
+ * to what the .tpl configuration file expects.
+ */
+#define WPP_RECORDER_FLAGS_LEVEL_ARGS(flags, lvl) \
+	WPP_RECORDER_LEVEL_FLAGS_ARGS(lvl, flags)
+#define WPP_RECORDER_FLAGS_LEVEL_FILTER(flags, lvl) \
+	WPP_RECORDER_LEVEL_FLAGS_FILTER(lvl, flags)
+
+/*
+begin_wpp config
+
+USEPREFIX(TraceError, "[%!FUNC!] ");
+FUNC TraceError{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_ERROR}(MSG, ...);
+
+USEPREFIX(TraceWarning, "[%!FUNC!] ");
+FUNC TraceWarning{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_WARNING}(MSG, ...);
+
+USEPREFIX(TraceInfo, "[%!FUNC!] ");
+FUNC TraceInfo{FLAGS=TRACE_GENERAL, LEVEL=TRACE_LEVEL_INFORMATION}(MSG, ...);
+
+end_wpp
+*/
-- 
2.29.3


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH v3 0/3] windows/virt2phys: fix paging issue
  2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
                       ` (2 preceding siblings ...)
  2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
@ 2022-01-11 13:56     ` Thomas Monjalon
  3 siblings, 0 replies; 21+ messages in thread
From: Thomas Monjalon @ 2022-01-11 13:56 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, Ranjit Menon

12/10/2021 02:42, Dmitry Kozlyuk:
> v3:
>     * Fix Release build (Ranjit).
>     * Drop PnpLockdown=1 patch as it is now in dependency series.
> v2:
>     * Following ofline review by DmitryM:
>       - Add comment explaining tracking approach for validation team.
>       - Replace deprecated allocation API calls.
>       - Check properties of locked memory (see docs in patch 3/4).
>       - Add configurable limits for tracked processes and memory.
>     * Add end-user documentation.
>     * Drop patch for Inf2Cat settings UseLocalTime=true:
>       the issue it resolves originated from development VM.
>     * Add PnpLockdown=1 patch.
> 
> 
> Dmitry Kozlyuk (3):
>   windows/virt2phys: do not expose pageable physical addresses
>   windows/virt2phys: add limits against resource exhaustion
>   windows/virt2phys: add tracing

I suppose we will never have a review from Microsoft, so
Applied, thanks.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue
  2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
                   ` (3 preceding siblings ...)
  2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
@ 2021-06-29  5:16 ` Ranjit Menon
  4 siblings, 0 replies; 21+ messages in thread
From: Ranjit Menon @ 2021-06-29  5:16 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Dmitry Malloy, Narcisa Ana Maria Vasile, Pallavi Kadam,
	Tyler Retzlaff, Nick Connolly


On 5/1/2021 10:18 AM, Dmitry Kozlyuk wrote:
> Physical addresses exposed by virt2phys driver could become pageable.
> This presents stability and security issues that prevent Microsoft
> from signing virt2phys, because a signed driver would be trusted
> by all end-user machines.
>
> Ensure that memory for which physical addresses are exposed by
> virt2phys is non-pageable at least for the lifetime of the process.
> As virt2phys code grows, make its development and debugging easier.
>
> There are other known issues that come from using PA and accessing DMA
> from userspace. They are not related to virt2phys par se. It is planned
> to address them later by enabling the use of IOMMU for DPDK on Windows.
>
> Dmitry Kozlyuk (3):
>    windows/virt2phys: use local time for signing
>    windows/virt2phys: do not expose pageable physical addresses
>    windows/virt2phys: add tracing
>
>   windows/virt2phys/virt2phys.c               |  89 ++++--
>   windows/virt2phys/virt2phys.vcxproj         |   8 +-
>   windows/virt2phys/virt2phys.vcxproj.filters |  11 +-
>   windows/virt2phys/virt2phys_logic.c         | 312 ++++++++++++++++++++
>   windows/virt2phys/virt2phys_logic.h         |  32 ++
>   windows/virt2phys/virt2phys_trace.h         |  49 +++
>   6 files changed, 472 insertions(+), 29 deletions(-)
>   create mode 100644 windows/virt2phys/virt2phys_logic.c
>   create mode 100644 windows/virt2phys/virt2phys_logic.h
>   create mode 100644 windows/virt2phys/virt2phys_trace.h

Acked-by: Ranjit Menon <ranjit.menon@intel.com>


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-01-11 13:56 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 17:18 [dpdk-dev] [kmods PATCH 0/3] windows/virt2phys: fix paging issue Dmitry Kozlyuk
2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 1/3] windows/virt2phys: use local time for signing Dmitry Kozlyuk
2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 2/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
2021-05-01 17:18 ` [dpdk-dev] [kmods PATCH 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
2021-05-26 21:01 ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Dmitry Kozlyuk
2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 1/4] windows/virt2phys: add PnpLockdown directive Dmitry Kozlyuk
2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 2/4] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 3/4] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
2021-05-26 21:01   ` [dpdk-dev] [kmods PATCH v2 4/4] windows/virt2phys: add tracing Dmitry Kozlyuk
2021-09-30 22:07     ` Menon, Ranjit
2021-09-30 22:13       ` Menon, Ranjit
2021-10-01  7:10       ` Dmitry Kozlyuk
2021-06-23  7:13   ` [dpdk-dev] [kmods PATCH v2 0/4] windows/virt2phys: fix paging issue Thomas Monjalon
2021-09-30 20:24     ` Thomas Monjalon
2021-09-30 20:39       ` Dmitry Kozlyuk
2021-10-12  0:42   ` [dpdk-dev] [kmods PATCH v3 0/3] " Dmitry Kozlyuk
2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 1/3] windows/virt2phys: do not expose pageable physical addresses Dmitry Kozlyuk
2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 2/3] windows/virt2phys: add limits against resource exhaustion Dmitry Kozlyuk
2021-10-12  0:42     ` [dpdk-dev] [kmods PATCH v3 3/3] windows/virt2phys: add tracing Dmitry Kozlyuk
2022-01-11 13:56     ` [dpdk-dev] [kmods PATCH v3 0/3] windows/virt2phys: fix paging issue Thomas Monjalon
2021-06-29  5:16 ` [dpdk-dev] [kmods PATCH " Ranjit Menon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).