From f07126913848e5c89e0bb0d3986e3bb13b78e3e7 Mon Sep 17 00:00:00 2001 From: Lev Stipakov Date: Wed, 15 Feb 2023 11:44:30 +0200 Subject: [PATCH] Do not use invalid queue handle on Rx notification When system goes to S3 (legacy standby), devices goes from D0 to D3 and NetAdapter destroys datapath, which destroys Tx/Rx queues. When system wakes up, NetAdapter recreates queues. While we update Rx queue handle in EvtAdapterCreateRxQueue, there is time frame between queue being destroyed and new queue is created. If we got incoming packet on the socket during that time frame, we inticate it to Rx queue and white doing that, we get queue's WDF context. When this is done with the handle of destroyed queue, this causes NULL pointer access inside WDF framework. Fix by assigning Rx queue handle to adapter context in EvtQueueStart callback and settings it to WDF_NO_HANDLE in EvtRxQueueStop. In OvpnAdapterNotifyRx proceed with Rx notification only if rxQueue handle is valid. Fixes https://github.com/OpenVPN/ovpn-dco-win/issues/36 Signed-off-by: Lev Stipakov --- Driver.cpp | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++- adapter.cpp | 21 ++++++++++++----- adapter.h | 2 +- rxqueue.cpp | 33 ++++++++++++++++++++++++++ rxqueue.h | 5 ++++ trace.h | 16 ++++++------- 6 files changed, 128 insertions(+), 16 deletions(-) diff --git a/Driver.cpp b/Driver.cpp index fe2e9b8..404d4f3 100644 --- a/Driver.cpp +++ b/Driver.cpp @@ -73,7 +73,9 @@ DriverEntry(_In_ PDRIVER_OBJECT driverObject, _In_ PUNICODE_STRING registryPath) GOTO_IF_NOT_NT_SUCCESS(done, status, TraceLoggingRegister(g_hOvpnEtwProvider)); traceLoggingRegistered = TRUE; - LOG_INFO("Driver Version", TraceLoggingValue(OVPN_DCO_VERSION_MAJOR, "Major"), TraceLoggingValue(OVPN_DCO_VERSION_MINOR, "Minor"), TraceLoggingValue(OVPN_DCO_VERSION_PATCH, "Patch")); + LOG_INFO("Driver Version", TraceLoggingValue(OVPN_DCO_VERSION_MAJOR, "Major"), + TraceLoggingValue(OVPN_DCO_VERSION_MINOR, "Minor"), + TraceLoggingValue(OVPN_DCO_VERSION_PATCH, "Patch")); WDF_OBJECT_ATTRIBUTES driverAttrs; WDF_OBJECT_ATTRIBUTES_INIT(&driverAttrs); @@ -313,6 +315,61 @@ VOID OvpnEvtDeviceCleanup(WDFOBJECT obj) { LOG_EXIT(); } +EVT_WDF_DEVICE_PREPARE_HARDWARE OvpnEvtDevicePrepareHardware; +EVT_WDF_DEVICE_RELEASE_HARDWARE OvpnEvtDeviceReleaseHardware; +_No_competing_thread_ EVT_WDF_DEVICE_D0_ENTRY OvpnEvtDeviceD0Entry; +_No_competing_thread_ EVT_WDF_DEVICE_D0_EXIT OvpnEvtDeviceD0Exit; + +_Use_decl_annotations_ +NTSTATUS +OvpnEvtDevicePrepareHardware(_In_ WDFDEVICE wdfDevice, _In_ WDFCMRESLIST resourcesRaw, _In_ WDFCMRESLIST resourcesTranslated) +{ + UNREFERENCED_PARAMETER(wdfDevice); + UNREFERENCED_PARAMETER(resourcesRaw); + UNREFERENCED_PARAMETER(resourcesTranslated); + + LOG_ENTER(); + LOG_EXIT(); + return STATUS_SUCCESS; +} + +_Use_decl_annotations_ +NTSTATUS +OvpnEvtDeviceReleaseHardware(_In_ WDFDEVICE wdfDevice, _In_ WDFCMRESLIST resourcesTranslated) +{ + UNREFERENCED_PARAMETER(wdfDevice); + UNREFERENCED_PARAMETER(resourcesTranslated); + + LOG_ENTER(); + LOG_EXIT(); + return STATUS_SUCCESS; +} + +_Use_decl_annotations_ +NTSTATUS +OvpnEvtDeviceD0Entry(_In_ WDFDEVICE wdfDevice, WDF_POWER_DEVICE_STATE previousState) +{ + UNREFERENCED_PARAMETER(wdfDevice); + + LOG_ENTER(TraceLoggingUInt32(previousState, "PreviousState")); + + LOG_EXIT(); + + return STATUS_SUCCESS; +} + +_Use_decl_annotations_ +NTSTATUS +OvpnEvtDeviceD0Exit(_In_ WDFDEVICE Device, _In_ WDF_POWER_DEVICE_STATE TargetState) +{ + UNREFERENCED_PARAMETER(Device); + + LOG_ENTER(TraceLoggingUInt32(TargetState, "TargetState")); + + LOG_EXIT(); + return STATUS_SUCCESS; +} + EVT_WDF_DRIVER_DEVICE_ADD OvpnEvtDeviceAdd; _Use_decl_annotations_ @@ -335,6 +392,14 @@ OvpnEvtDeviceAdd(WDFDRIVER wdfDriver, PWDFDEVICE_INIT deviceInit) { NTSTATUS status; GOTO_IF_NOT_NT_SUCCESS(done, status, NetDeviceInitConfig(deviceInit)); + WDF_PNPPOWER_EVENT_CALLBACKS pnpPowerCallbacks; + WDF_PNPPOWER_EVENT_CALLBACKS_INIT(&pnpPowerCallbacks); + pnpPowerCallbacks.EvtDevicePrepareHardware = OvpnEvtDevicePrepareHardware; + pnpPowerCallbacks.EvtDeviceReleaseHardware = OvpnEvtDeviceReleaseHardware; + pnpPowerCallbacks.EvtDeviceD0Entry = OvpnEvtDeviceD0Entry; + pnpPowerCallbacks.EvtDeviceD0Exit = OvpnEvtDeviceD0Exit; + WdfDeviceInitSetPnpPowerEventCallbacks(deviceInit, &pnpPowerCallbacks); + WDF_OBJECT_ATTRIBUTES objAttributes; WDF_OBJECT_ATTRIBUTES_INIT_CONTEXT_TYPE(&objAttributes, OVPN_DEVICE); // BCryptOpenAlgorithmProvider with BCRYPT_PROV_DISPATCH returns STATUS_NOT_SUPPORTED if sync scope is WdfSynchronizationScopeDevice diff --git a/adapter.cpp b/adapter.cpp index 6614170..e8ece3b 100644 --- a/adapter.cpp +++ b/adapter.cpp @@ -132,11 +132,17 @@ _Use_decl_annotations_ NTSTATUS OvpnEvtAdapterCreateRxQueue(NETADAPTER netAdapter, NETRXQUEUE_INIT* rxQueueInit) { + LOG_ENTER(); + WDF_OBJECT_ATTRIBUTES rxAttributes; NET_PACKET_QUEUE_CONFIG rxConfig; WDF_OBJECT_ATTRIBUTES_INIT_CONTEXT_TYPE(&rxAttributes, OVPN_RXQUEUE); + rxAttributes.EvtDestroyCallback = OvpnEvtRxQueueDestroy; + NET_PACKET_QUEUE_CONFIG_INIT(&rxConfig, OvpnEvtRxQueueAdvance, OvpnEvtRxQueueSetNotificationEnabled, OvpnEvtRxQueueCancel); + rxConfig.EvtStart = OvpnEvtRxQueueStart; + rxConfig.EvtStop = OvpnEvtRxQueueStop; NETPACKETQUEUE netPacketQueue; NTSTATUS status; @@ -144,9 +150,10 @@ OvpnEvtAdapterCreateRxQueue(NETADAPTER netAdapter, NETRXQUEUE_INIT* rxQueueInit) POVPN_ADAPTER adapterContext = OvpnGetAdapterContext(netAdapter); OvpnRxQueueInitialize(netPacketQueue, adapterContext); - adapterContext->RxQueue = netPacketQueue; done: + LOG_EXIT(); + return status; } @@ -207,18 +214,20 @@ OvpnAdapterCreate(OVPN_DEVICE * device) { return status; } -NTSTATUS OvpnAdapterNotifyRx(NETADAPTER netAdapter) +VOID OvpnAdapterNotifyRx(NETADAPTER netAdapter) { if (netAdapter == WDF_NO_HANDLE) { LOG_ERROR("Adapter not initialized"); - return STATUS_DEVICE_NOT_READY; + return; } NETPACKETQUEUE rxQueue = OvpnGetAdapterContext(netAdapter)->RxQueue; - POVPN_RXQUEUE queueContext = OvpnGetRxQueueContext(rxQueue); + if (rxQueue == WDF_NO_HANDLE) { + LOG_WARN("rxQueue not initialized"); + return; + } + POVPN_RXQUEUE queueContext = OvpnGetRxQueueContext(rxQueue); if (InterlockedExchange(&queueContext->NotificationEnabled, FALSE) == TRUE) NetRxQueueNotifyMoreReceivedPacketsAvailable(rxQueue); - - return STATUS_SUCCESS; } diff --git a/adapter.h b/adapter.h index 2eb0a3a..adef4c8 100644 --- a/adapter.h +++ b/adapter.h @@ -49,7 +49,7 @@ NTSTATUS OvpnAdapterCreate(OVPN_DEVICE* device); // notify NetAdapter (if it is ready) that more packets are available -NTSTATUS +VOID OvpnAdapterNotifyRx(NETADAPTER netAdapter); VOID diff --git a/rxqueue.cpp b/rxqueue.cpp index afda2f2..bc0f8c0 100644 --- a/rxqueue.cpp +++ b/rxqueue.cpp @@ -29,9 +29,42 @@ #include "bufferpool.h" #include "rxqueue.h" #include "netringiterator.h" +#include "trace.h" EVT_PACKET_QUEUE_ADVANCE OvpnEvtRxQueueAdvance; +_Use_decl_annotations_ +VOID +OvpnEvtRxQueueStart(NETPACKETQUEUE netPacketQueue) +{ + LOG_ENTER(TraceLoggingPointer(netPacketQueue, "RxQueue")); + + POVPN_RXQUEUE queue = OvpnGetRxQueueContext(netPacketQueue); + queue->Adapter->RxQueue = netPacketQueue; + + LOG_EXIT(); +} + +_Use_decl_annotations_ +VOID +OvpnEvtRxQueueStop(NETPACKETQUEUE netPacketQueue) +{ + LOG_ENTER(TraceLoggingPointer(netPacketQueue, "RxQueue")); + + POVPN_RXQUEUE queue = OvpnGetRxQueueContext(netPacketQueue); + queue->Adapter->RxQueue = WDF_NO_HANDLE; + + LOG_EXIT(); +} + +_Use_decl_annotations_ +VOID +OvpnEvtRxQueueDestroy(WDFOBJECT rxQueue) +{ + LOG_ENTER(TraceLoggingPointer(rxQueue, "RxQueue")); + LOG_EXIT(); +} + static inline UINT8 OvpnRxQueueGetLayer4Type(const VOID* buf, size_t len) { diff --git a/rxqueue.h b/rxqueue.h index 8a5e1de..a00ab7c 100644 --- a/rxqueue.h +++ b/rxqueue.h @@ -41,5 +41,10 @@ EVT_PACKET_QUEUE_SET_NOTIFICATION_ENABLED OvpnEvtRxQueueSetNotificationEnabled; EVT_PACKET_QUEUE_ADVANCE OvpnEvtRxQueueAdvance; EVT_PACKET_QUEUE_CANCEL OvpnEvtRxQueueCancel; +EVT_PACKET_QUEUE_START OvpnEvtRxQueueStart; +EVT_PACKET_QUEUE_STOP OvpnEvtRxQueueStop; + +EVT_WDF_OBJECT_CONTEXT_DESTROY OvpnEvtRxQueueDestroy; + VOID OvpnRxQueueInitialize(NETPACKETQUEUE rxQueue, _In_ POVPN_ADAPTER adapter); diff --git a/trace.h b/trace.h index 6d7bb68..eb85547 100644 --- a/trace.h +++ b/trace.h @@ -65,24 +65,24 @@ TRACELOGGING_DECLARE_PROVIDER(g_hOvpnEtwProvider); __VA_ARGS__); \ } while (0,0) -#define LOG_ENTER() do {\ +#define LOG_ENTER(...) do {\ TraceLoggingWrite( \ g_hOvpnEtwProvider, \ - "Info", \ - TraceLoggingLevel(TRACE_LEVEL_INFORMATION), \ + "FunctionEntry", \ + TraceLoggingLevel(TRACE_LEVEL_VERBOSE), \ TraceLoggingFunctionName(), \ TraceLoggingUInt32(__LINE__, "Line"), \ - TraceLoggingValue("Enter", "Msg")); \ + __VA_ARGS__); \ } while (0,0) -#define LOG_EXIT() do {\ +#define LOG_EXIT(...) do {\ TraceLoggingWrite( \ g_hOvpnEtwProvider, \ - "Info", \ - TraceLoggingLevel(TRACE_LEVEL_INFORMATION), \ + "FunctionExit", \ + TraceLoggingLevel(TRACE_LEVEL_VERBOSE), \ TraceLoggingFunctionName(), \ TraceLoggingUInt32(__LINE__, "Line"), \ - TraceLoggingValue("Exit", "Msg")); \ + __VA_ARGS__); \ } while (0,0) #define LOG_INFO(Info, ...) do {\