From 0a545d04f7b5102c7b43fbae0a39efe70cfc8180 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20H=C3=B6glinger-Stelzer?= Date: Thu, 2 Jul 2026 20:57:06 +0200 Subject: [PATCH] Stability fixes --- src/Dmf_BusFilter.c | 460 ++++++++++++++++++++++++++++++++++++-------- src/Dmf_BusFilter.h | 6 + 2 files changed, 387 insertions(+), 79 deletions(-) diff --git a/src/Dmf_BusFilter.c b/src/Dmf_BusFilter.c index 3dc26e9..431fe2a 100644 --- a/src/Dmf_BusFilter.c +++ b/src/Dmf_BusFilter.c @@ -129,6 +129,42 @@ WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(BusFilter_Context, DEFINE_GUID(GUID_DMF_BUSFILTER_SIGNATURE, 0x678cbb8d, 0x19f, 0x4d07, 0x91, 0x2a, 0x73, 0xe2, 0xe5, 0x68, 0xb1, 0x48); +// Pool tag for allocations made by this module. +// +#define DMF_BUSFILTER_POOL_TAG 'tlfB' + +// Context passed to the passive-level work item used when the bus-relations +// completion routine fires at DISPATCH_LEVEL (Finding 3). +// +typedef struct _BUS_RELATIONS_WORK_CONTEXT +{ + PIO_WORKITEM WorkItem; + WDFDEVICE Device; + PIRP Irp; +} BUS_RELATIONS_WORK_CONTEXT; + +#pragma code_seg("PAGE") +_IRQL_requires_max_(PASSIVE_LEVEL) +static +void +DMF_BusFilter_Relations_RemoveDevice( + _In_ PDEVICE_OBJECT DeviceObject + ); +#pragma code_seg() + +_IRQL_requires_max_(PASSIVE_LEVEL) +static +NTSTATUS +DMF_BusFilter_Relations_AddDevice( + _In_ WDFDEVICE Device, + _In_ PDEVICE_OBJECT PhysicalDeviceObject + ); + +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// IRP_MN_REMOVE_DEVICE teardown +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// + #pragma code_seg("PAGE") _IRQL_requires_max_(PASSIVE_LEVEL) static @@ -140,11 +176,12 @@ DMF_BusFilter_Relations_RemoveDevice( Routine Description: - Processes child device removal. + Processes child device removal. Always performs a full teardown regardless of + the IsExisting flag so that surprise removals are handled correctly. Arguments: - DeviceObject - Parent device object. + DeviceObject - Filter device object whose stack is being torn down. Return Value: @@ -162,11 +199,10 @@ Return Value: PAGED_CODE(); - if (extension->IsExisting) - { - goto Exit; - } - + // Always remove from list and tear down on REMOVE_DEVICE, regardless of + // IsExisting state. A surprise removal can arrive while IsExisting is TRUE, + // and skipping teardown leaves the filter attached to a dead stack. + // #pragma warning(disable: 28150) // elevates to DISPATCH_LEVEL KeAcquireInStackQueuedSpinLock(&parentContext->ChildListLock, &handle); @@ -185,12 +221,15 @@ Return Value: IoDetachDevice(extension->TargetDeviceObject); IoDeleteDevice(DeviceObject); -Exit: - FuncExitNoReturn(DMF_TRACE); } #pragma code_seg() +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// Child PnP dispatch +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// + _IRQL_requires_max_(DISPATCH_LEVEL) static NTSTATUS @@ -203,13 +242,13 @@ DMF_BusFilter_DispatchPnp( Routine Description: - Handles PnP requests. + Handles PnP requests for child filter devices. Arguments: - DeviceObject - Parent device object. - Irp - Irp with PnP request. - MinorCode - Request minor code. + DeviceObject - Child filter device object. + Irp - Irp with PnP request. + MinorCode - PnP request minor code. Return Value: @@ -217,28 +256,48 @@ Return Value: --*/ { + NTSTATUS ntStatus; const WDM_CHILD_DEVICE_EXTENSION* extension = (WDM_CHILD_DEVICE_EXTENSION*)DeviceObject->DeviceExtension; const BusFilter_Context* context = BusFilterContextGet(WdfGetDriver()); if (MinorCode == IRP_MN_REMOVE_DEVICE) { - // Handle child device removal - // + // WDM filter driver guidance: forward the IRP to the lower driver FIRST, + // then perform our own teardown. Saving TargetDeviceObject to a local + // before teardown prevents a use-after-free: Relations_RemoveDevice calls + // IoDeleteDevice, making extension memory invalid before we could forward. + // + // PnP IRPs always arrive at PASSIVE_LEVEL so IoForwardIrpSynchronously + // is safe here even though the function is annotated DISPATCH_LEVEL max + // for other minor codes. + // + PDEVICE_OBJECT targetDevice = extension->TargetDeviceObject; + + if (!IoForwardIrpSynchronously(targetDevice, Irp)) + { + Irp->IoStatus.Status = STATUS_NO_SUCH_DEVICE; + } + + ntStatus = Irp->IoStatus.Status; + IoCompleteRequest(Irp, IO_NO_INCREMENT); + + // Tear down now that the IRP has been forwarded and completed. + // DMF_BusFilter_Relations_RemoveDevice(DeviceObject); + + return ntStatus; } - else if (MinorCode <= IRP_MN_DEVICE_ENUMERATED) + + if (MinorCode <= IRP_MN_DEVICE_ENUMERATED) { if (context->PnPMinorDispatchFunctions[MinorCode] != NULL) { - // - // Forward to PnP minor code dispatch routines - // return context->PnPMinorDispatchFunctions[MinorCode](extension->Child, Irp); } } // Forward to lower driver - // + // IoSkipCurrentIrpStackLocation(Irp); return IoCallDriver(extension->TargetDeviceObject, Irp); @@ -291,7 +350,17 @@ Return Value: return IoCallDriver(extension->TargetDeviceObject, Irp); } -_IRQL_requires_max_(APC_LEVEL) +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// Child device creation +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// + +// IoCreateDevice and IoAttachDeviceToDeviceStack both require PASSIVE_LEVEL. +// The annotation is corrected from the original APC_LEVEL. +// Note: this function is NOT in the PAGE segment because it acquires a spinlock +// (which momentarily elevates to DISPATCH_LEVEL), making paged memory unsafe. +// +_IRQL_requires_max_(PASSIVE_LEVEL) static NTSTATUS DMF_BusFilter_Relations_AddDevice( @@ -306,8 +375,8 @@ Routine Description: Arguments: - Device - Child device to add. - PhysicalDeviceObject - Parent device object. + Device - Parent WDF bus filter device. + PhysicalDeviceObject - Bus PDO to filter. Return Value: @@ -417,6 +486,7 @@ Return Value: if (childExtension->TargetDeviceObject == NULL) { IoDeleteDevice(filterDeviceObject); + filterDeviceObject = NULL; ntStatus = STATUS_NO_SUCH_DEVICE; goto Exit; } @@ -434,17 +504,56 @@ Return Value: IoDetachDevice(childExtension->TargetDeviceObject); IoDeleteDevice(filterDeviceObject); + filterDeviceObject = NULL; goto Exit; } } KeAcquireInStackQueuedSpinLock(&parentContext->ChildListLock, &handle); - childExtension->IsExisting = TRUE; - InsertTailList(&parentContext->ChildList, - &childExtension->ListEntry); + + // Re-scan under the lock before inserting to guard against a duplicate created + // by a concurrent bus-relations completion (Finding 5). + // + BOOLEAN duplicate = FALSE; + for ( + PLIST_ENTRY scanEntry = parentContext->ChildList.Flink; + scanEntry != &parentContext->ChildList; + scanEntry = scanEntry->Flink + ) + { + WDM_CHILD_DEVICE_EXTENSION* scanExt = CONTAINING_RECORD(scanEntry, + WDM_CHILD_DEVICE_EXTENSION, + ListEntry); + if (scanExt->PhysicalDeviceObject == PhysicalDeviceObject) + { + duplicate = TRUE; + break; + } + } + + if (!duplicate) + { + childExtension->IsExisting = TRUE; + InsertTailList(&parentContext->ChildList, + &childExtension->ListEntry); + } + KeReleaseInStackQueuedSpinLock(&handle); + if (duplicate) + { + // A concurrent AddDevice beat us to it; discard our newly created device. + // + IoDetachDevice(childExtension->TargetDeviceObject); + IoDeleteDevice(filterDeviceObject); + filterDeviceObject = NULL; + WdfObjectDelete(child); + child = NULL; + ntStatus = STATUS_SUCCESS; + goto Exit; + } + filterDeviceObject->Flags &= ~DO_DEVICE_INITIALIZING; ntStatus = STATUS_SUCCESS; @@ -461,66 +570,41 @@ Exit: return ntStatus; } +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// Bus-relations completion: passive-level processing +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// + +// Helper that resets IsExisting flags and calls AddDevice for every PDO in the +// DEVICE_RELATIONS returned by the lower bus driver. Must be called at PASSIVE_LEVEL +// because DMF_BusFilter_Relations_AddDevice requires it. +// #pragma code_seg("PAGE") _IRQL_requires_max_(PASSIVE_LEVEL) static -NTSTATUS -DMF_BusFilter_QueryBusRelationsCompleted( - _In_ PDEVICE_OBJECT DeviceObject, - _In_ PIRP Irp, - _In_ WDFDEVICE Device +void +DMF_BusFilter_ProcessBusRelations( + _In_ WDFDEVICE Device, + _In_ PIRP Irp ) -/*++ - -Routine Description: - - Bus relations query completed routine. - -Arguments: - - DeviceObject - Parent device object. - Irp - Query Bus Relations IRP. - Device - Target WDFDEVICE. - -Return Value: - - NTSTATUS - ---*/ { - NTSTATUS ntStatus = STATUS_NOT_IMPLEMENTED; - PARENT_BUS_DEVICE_CONTEXT* parentContext = DMF_BusFilter_GetParentContext(Device); - PDEVICE_RELATIONS deviceRelations = NULL; KLOCK_QUEUE_HANDLE handle; WDM_CHILD_DEVICE_EXTENSION* childExtension = NULL; - - UNREFERENCED_PARAMETER(DeviceObject); - - FuncEntry(DMF_TRACE); + PARENT_BUS_DEVICE_CONTEXT* parentContext = DMF_BusFilter_GetParentContext(Device); PAGED_CODE(); - if (Irp->PendingReturned) - { - IoMarkIrpPending(Irp); - } - - if (!NT_SUCCESS(Irp->IoStatus.Status)) - { - goto Exit; - } - if (parentContext == NULL) { - goto Exit; + return; } -#pragma warning(disable: 28150) // elevates to DISPATCH_LEVEL +#pragma warning(disable: 28150) KeAcquireInStackQueuedSpinLock(&parentContext->ChildListLock, &handle); - // Reset child states - // + // Reset child states so stale entries can be detected later + // for ( LIST_ENTRY* entry = parentContext->ChildList.Flink; entry != &parentContext->ChildList; @@ -533,29 +617,149 @@ Return Value: childExtension->IsExisting = FALSE; } - KeReleaseInStackQueuedSpinLock(&handle); // drops to PASSIVE_LEVEL + KeReleaseInStackQueuedSpinLock(&handle); #pragma warning(default: 28150) - deviceRelations = (PDEVICE_RELATIONS)Irp->IoStatus.Information; + const PDEVICE_RELATIONS deviceRelations = (PDEVICE_RELATIONS)Irp->IoStatus.Information; if (deviceRelations == NULL) { - goto Exit; + return; } - // Walk through device relations. - // for (ULONG index = 0; index < deviceRelations->Count; index++) - { - TraceVerbose(DMF_TRACE, "%!FUNC! called at %!irql!", KeGetCurrentIrql()); + { + TraceVerbose(DMF_TRACE, "%!FUNC! called at %!irql!", KeGetCurrentIrql()); - ntStatus = DMF_BusFilter_Relations_AddDevice(Device, - deviceRelations->Objects[index]); + NTSTATUS ntStatus = DMF_BusFilter_Relations_AddDevice(Device, + deviceRelations->Objects[index]); if (!NT_SUCCESS(ntStatus)) { TraceError(DMF_TRACE, "DMF_BusFilter_Relations_AddDevice fails: ntStatus=%!STATUS!", ntStatus); } } +} +#pragma code_seg() + +// Work item routine: runs at PASSIVE_LEVEL when the completion routine fires at +// DISPATCH_LEVEL and cannot call IoCreateDevice / IoAttachDeviceToDeviceStack +// directly (Finding 3). +// +static +IO_WORKITEM_ROUTINE DMF_BusFilter_QueryBusRelationsWorker; + +static +void +DMF_BusFilter_QueryBusRelationsWorker( + _In_ PDEVICE_OBJECT DeviceObject, + _In_opt_ PVOID Context + ) +{ + UNREFERENCED_PARAMETER(DeviceObject); + + BUS_RELATIONS_WORK_CONTEXT* workCtx = (BUS_RELATIONS_WORK_CONTEXT*)Context; + + if (workCtx == NULL) + { + return; + } + + PIRP irp = workCtx->Irp; + + DMF_BusFilter_ProcessBusRelations(workCtx->Device, irp); + + IoFreeWorkItem(workCtx->WorkItem); + ExFreePoolWithTag(workCtx, DMF_BUSFILTER_POOL_TAG); + + // Resume IRP completion propagation upward through the stack. The completion + // routine returned STATUS_MORE_PROCESSING_REQUIRED which halted it at our level. + // + IoCompleteRequest(irp, IO_NO_INCREMENT); +} + +// Completion routine for IRP_MN_QUERY_DEVICE_RELATIONS / BusRelations. +// +// This routine is intentionally NOT in the PAGE segment and does NOT call +// PAGED_CODE() because IO completion routines may run at DISPATCH_LEVEL. +// When that happens the passive-level AddDevice work is deferred to a work item. +// +_IRQL_requires_max_(DISPATCH_LEVEL) +static +NTSTATUS +DMF_BusFilter_QueryBusRelationsCompleted( + _In_ PDEVICE_OBJECT DeviceObject, + _In_ PIRP Irp, + _In_ WDFDEVICE Device + ) +/*++ + +Routine Description: + + Bus relations query completion routine. + +Arguments: + + DeviceObject - Parent device object (unused). + Irp - Query Bus Relations IRP. + Device - Target WDFDEVICE. + +Return Value: + + STATUS_CONTINUE_COMPLETION or STATUS_MORE_PROCESSING_REQUIRED + +--*/ +{ + UNREFERENCED_PARAMETER(DeviceObject); + + FuncEntry(DMF_TRACE); + + if (Irp->PendingReturned) + { + IoMarkIrpPending(Irp); + } + + if (!NT_SUCCESS(Irp->IoStatus.Status)) + { + goto Exit; + } + + // If completion is running at DISPATCH_LEVEL, IoCreateDevice and + // IoAttachDeviceToDeviceStack cannot be called safely. Defer to a + // passive-level work item. + // + if (KeGetCurrentIrql() > PASSIVE_LEVEL) + { + BUS_RELATIONS_WORK_CONTEXT* workCtx = + (BUS_RELATIONS_WORK_CONTEXT*)ExAllocatePoolWithTag(NonPagedPool, + sizeof(BUS_RELATIONS_WORK_CONTEXT), + DMF_BUSFILTER_POOL_TAG); + if (workCtx != NULL) + { + workCtx->Device = Device; + workCtx->Irp = Irp; + workCtx->WorkItem = IoAllocateWorkItem(WdfDeviceWdmGetDeviceObject(Device)); + + if (workCtx->WorkItem != NULL) + { + IoQueueWorkItem(workCtx->WorkItem, + DMF_BusFilter_QueryBusRelationsWorker, + DelayedWorkQueue, + workCtx); + + FuncExitNoReturn(DMF_TRACE); + return STATUS_MORE_PROCESSING_REQUIRED; + } + + ExFreePoolWithTag(workCtx, DMF_BUSFILTER_POOL_TAG); + } + + TraceError(DMF_TRACE, "Failed to allocate work item for bus relations at DISPATCH_LEVEL; skipping child creation"); + goto Exit; + } + + // PASSIVE_LEVEL path: process child devices inline. + // + DMF_BusFilter_ProcessBusRelations(Device, Irp); Exit: @@ -563,7 +767,6 @@ Exit: return STATUS_CONTINUE_COMPLETION; } -#pragma code_seg() _IRQL_requires_max_(DISPATCH_LEVEL) static @@ -614,6 +817,11 @@ Return Value: Irp); } +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// PnP minor-code handlers +/////////////////////////////////////////////////////////////////////////////////////////////////////// +// + #pragma code_seg("PAGE") static NTSTATUS @@ -1003,7 +1211,16 @@ Return Value: PAGED_CODE(); + // Guard against DMF_BusFilter_Initialize not having been called. + // const BusFilter_Context* context = BusFilterContextGet(Driver); + if (context == NULL) + { + TraceError(DMF_TRACE, "BusFilterContextGet returns NULL: was DMF_BusFilter_Initialize called?"); + ntStatus = STATUS_INVALID_DEVICE_STATE; + goto Exit; + } + const DMF_BusFilter_CONFIG* config = &context->Configuration; WdfFdoInitSetFilter(DeviceInit); @@ -1210,4 +1427,89 @@ Return Value: return NULL; } +#pragma code_seg("PAGE") +_IRQL_requires_max_(PASSIVE_LEVEL) +VOID +DMF_BusFilter_DeviceCleanup( + _In_ WDFDEVICE Device + ) +/*++ + +Routine Description: + + Tears down all remaining child filter devices associated with a bus device. + The client driver should call this from its EvtDeviceReleaseHardware or + EvtCleanupCallback to ensure WDM filter devices are not leaked on unload. + Without this call, any child devices still in ChildList at unload time are + leaked and will fault when the I/O manager later dereferences them. + +Arguments: + + Device - The bus WDF device whose children should be cleaned up. + +Return Value: + + None + +--*/ +{ + KLOCK_QUEUE_HANDLE handle; + PARENT_BUS_DEVICE_CONTEXT* parentContext = DMF_BusFilter_GetParentContext(Device); + const BusFilter_Context* context = BusFilterContextGet(WdfGetDriver()); + + FuncEntry(DMF_TRACE); + + PAGED_CODE(); + + if (parentContext == NULL || context == NULL) + { + goto Exit; + } + + // Drain ChildList, detaching and deleting each filter device. + // The lock is released around the passive-level teardown calls because + // IoDetachDevice and IoDeleteDevice cannot be called while holding a spinlock. + // +#pragma warning(disable: 28150) + KeAcquireInStackQueuedSpinLock(&parentContext->ChildListLock, &handle); + + while (!IsListEmpty(&parentContext->ChildList)) + { + LIST_ENTRY* entry = RemoveHeadList(&parentContext->ChildList); + + KeReleaseInStackQueuedSpinLock(&handle); + + WDM_CHILD_DEVICE_EXTENSION* childExtension = + CONTAINING_RECORD(entry, WDM_CHILD_DEVICE_EXTENSION, ListEntry); + + PDEVICE_OBJECT filterDevice = DMF_BusFilter_WdmDeviceObjectGet(childExtension->Child); + PDEVICE_OBJECT targetDevice = childExtension->TargetDeviceObject; + DMFBUSCHILDDEVICE child = childExtension->Child; + WDFDEVICE parent = childExtension->Parent; + + if (context->Configuration.EvtDeviceRemove) + { + context->Configuration.EvtDeviceRemove(parent, child); + } + + WdfObjectDelete(child); + IoDetachDevice(targetDevice); + + if (filterDevice != NULL) + { + IoDeleteDevice(filterDevice); + } + + KeAcquireInStackQueuedSpinLock(&parentContext->ChildListLock, &handle); + } + + KeReleaseInStackQueuedSpinLock(&handle); +#pragma warning(default: 28150) + +Exit: + + FuncExitNoReturn(DMF_TRACE); +} +#pragma code_seg() + #endif // defined(DMF_KERNEL_MODE) diff --git a/src/Dmf_BusFilter.h b/src/Dmf_BusFilter.h index 37d4981..da8c4d7 100644 --- a/src/Dmf_BusFilter.h +++ b/src/Dmf_BusFilter.h @@ -195,4 +195,10 @@ DMF_BusFilter_WdmPhysicalDeviceGet( _In_ DMFBUSCHILDDEVICE ChildDevice ); +_IRQL_requires_max_(PASSIVE_LEVEL) +VOID +DMF_BusFilter_DeviceCleanup( + _In_ WDFDEVICE Device + ); + #endif // defined(DMF_KERNEL_MODE)