From d8fe1419fb480ec5fc4dd0c4bc49f1d1a1a90663 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Tue, 12 Oct 2021 18:44:42 +0000 Subject: [PATCH] driver: automatically close long-lived handle There's only one handle that's likely to be open in a long lived way: the tun registration handle. So we can force that closed automatically when the device is about to close, if it's been improperly left open. Other handles will indeed hold up closing, but if those exist, they're a sign of a larger bug elsewhere that should be addressed. On the other hand, tun registration handles might legitimately be open during driver upgrades. This also saves us the trouble of dereferencing a freed FileObject as in the general case. Signed-off-by: Jason A. Donenfeld --- api/adapter.c | 43 -------------- api/adapter.h | 15 ----- api/driver.c | 4 -- driver/wintun.c | 147 +++++++++++++++++++++++++----------------------- 4 files changed, 77 insertions(+), 132 deletions(-) diff --git a/api/adapter.c b/api/adapter.c index b71032a..6aa6c21 100644 --- a/api/adapter.c +++ b/api/adapter.c @@ -191,8 +191,6 @@ WintunCloseAdapter(WINTUN_ADAPTER *Adapter) if (!Adapter) return; Free(Adapter->InterfaceFilename); - if (Adapter->DevInfo) - AdapterForceCloseHandles(Adapter->DevInfo, &Adapter->DevInfoData); if (Adapter->SwDevice) SwDeviceClose(Adapter->SwDevice); if (Adapter->DevInfo) @@ -896,47 +894,6 @@ cleanup: return RET_ERROR(Adapter, LastError); } -#define TUN_IOCTL_FORCE_CLOSE_HANDLES CTL_CODE(51820U, 0x971U, METHOD_NEITHER, FILE_READ_DATA | FILE_WRITE_DATA) - -_Use_decl_annotations_ -BOOL -AdapterForceCloseHandles(HDEVINFO DevInfo, SP_DEVINFO_DATA *DevInfoData) -{ - DWORD LastError = ERROR_SUCCESS; - WCHAR InstanceId[MAX_INSTANCE_ID]; - DWORD RequiredChars = _countof(InstanceId); - if (!SetupDiGetDeviceInstanceIdW(DevInfo, DevInfoData, InstanceId, RequiredChars, &RequiredChars)) - { - LOG_LAST_ERROR(L"Failed to get adapter instance ID"); - return FALSE; - } - WINTUN_ADAPTER Adapter = { .InterfaceFilename = AdapterGetDeviceObjectFileName(InstanceId) }; - if (!Adapter.InterfaceFilename) - { - LOG_LAST_ERROR(L"Failed to get adapter file name"); - return FALSE; - } - HANDLE Handle = AdapterOpenDeviceObject(&Adapter); - Free(Adapter.InterfaceFilename); - if (Handle == INVALID_HANDLE_VALUE) - { - LastError = LOG(WINTUN_LOG_ERR, L"Failed to get adapter file object"); - return FALSE; - } - DWORD RequiredBytes; - if (DeviceIoControl(Handle, TUN_IOCTL_FORCE_CLOSE_HANDLES, NULL, 0, NULL, 0, &RequiredBytes, NULL)) - { - LastError = ERROR_SUCCESS; - Sleep(200); - } - else if (GetLastError() == ERROR_NOTHING_TO_TERMINATE) - LastError = ERROR_SUCCESS; - else - LastError = LOG_LAST_ERROR(L"Failed to perform force close ioctl"); - CloseHandle(Handle); - return RET_ERROR(TRUE, LastError); -} - _Use_decl_annotations_ BOOL AdapterRemoveInstance(HDEVINFO DevInfo, SP_DEVINFO_DATA *DevInfoData) diff --git a/api/adapter.h b/api/adapter.h index ec84d70..567ee82 100644 --- a/api/adapter.h +++ b/api/adapter.h @@ -135,18 +135,3 @@ AdapterEnableInstance(_In_ HDEVINFO DevInfo, _In_ SP_DEVINFO_DATA *DevInfoData); _Return_type_success_(return != FALSE) BOOL AdapterDisableInstance(_In_ HDEVINFO DevInfo, _In_ SP_DEVINFO_DATA *DevInfoData); - -/** - * Force closes all device handles of the specified device instance. - * - * @param DevInfo Device info handle from SetupAPI. - * @param DevInfoData Device info data specifying which device. - * - * @return If the function succeeds, the return value is TRUE. If the - * function fails, the return value is FALSE. To get extended - * error information, call GetLastError. - */ - -_Return_type_success_(return != FALSE) -BOOL -AdapterForceCloseHandles(_In_ HDEVINFO DevInfo, _In_ SP_DEVINFO_DATA *DevInfoData); diff --git a/api/driver.c b/api/driver.c index 3fb4909..172749f 100644 --- a/api/driver.c +++ b/api/driver.c @@ -69,10 +69,6 @@ DisableAllOurAdapters(_In_ HDEVINFO DevInfo, _Inout_ SP_DEVINFO_DATA_LIST **Disa ((Status & DN_HAS_PROBLEM) && ProblemCode == CM_PROB_DISABLED)) goto cleanupDeviceNode; - LOG(WINTUN_LOG_INFO, L"Force closing adapter \"%s\" open handles", Name); - if (!AdapterForceCloseHandles(DevInfo, &DeviceNode->Data)) - LOG(WINTUN_LOG_WARN, L"Failed to force close adapter \"%s\" open handles", Name); - LOG(WINTUN_LOG_INFO, L"Disabling adapter \"%s\"", Name); if (!AdapterDisableInstance(DevInfo, &DeviceNode->Data)) { diff --git a/driver/wintun.c b/driver/wintun.c index 2e740e2..1969854 100644 --- a/driver/wintun.c +++ b/driver/wintun.c @@ -122,8 +122,6 @@ typedef struct _TUN_REGISTER_RINGS_32 * The lpInBuffer and nInBufferSize parameters of DeviceIoControl() must point to an TUN_REGISTER_RINGS struct. * Client must wait for this IOCTL to finish before adding packets to the ring. */ #define TUN_IOCTL_REGISTER_RINGS CTL_CODE(51820U, 0x970U, METHOD_BUFFERED, FILE_READ_DATA | FILE_WRITE_DATA) -/* Force close all open handles to allow for updating. */ -#define TUN_IOCTL_FORCE_CLOSE_HANDLES CTL_CODE(51820U, 0x971U, METHOD_NEITHER, FILE_READ_DATA | FILE_WRITE_DATA) typedef struct _TUN_CTX { @@ -181,7 +179,7 @@ typedef struct _TUN_CTX static UINT NdisVersion; static NDIS_HANDLE NdisMiniportDriverHandle; -static DRIVER_DISPATCH *NdisDispatchDeviceControl, *NdisDispatchClose; +static DRIVER_DISPATCH *NdisDispatchDeviceControl, *NdisDispatchClose, *NdisDispatchPnp; static ERESOURCE TunDispatchCtxGuard, TunDispatchDeviceListLock; static RTL_STATIC_LIST_HEAD(TunDispatchDeviceList); /* Binary representation of O:SYD:P(A;;FA;;;SY)(A;;FA;;;BA)S:(ML;;NWNRNX;;;HI) */ @@ -783,68 +781,6 @@ TunUnregisterBuffers(_Inout_ TUN_CTX *Ctx, _In_ FILE_OBJECT *Owner) ExReleaseResourceLite(&Ctx->Device.RegistrationLock); } -_IRQL_requires_max_(PASSIVE_LEVEL) -static BOOLEAN -TunForceHandlesClosed(_Inout_ DEVICE_OBJECT *DeviceObject) -{ - NTSTATUS Status; - PEPROCESS Process; - KAPC_STATE ApcState; - PVOID Object = NULL; - ULONG VerifierFlags = 0; - OBJECT_HANDLE_INFORMATION HandleInfo; - SYSTEM_HANDLE_INFORMATION_EX *HandleTable = NULL; - BOOLEAN DidClose = FALSE; - - MmIsVerifierEnabled(&VerifierFlags); - - for (ULONG Size = 0, RequestedSize; - (Status = ZwQuerySystemInformation(SystemExtendedHandleInformation, HandleTable, Size, &RequestedSize)) == - STATUS_INFO_LENGTH_MISMATCH; - Size = RequestedSize) - { - if (HandleTable) - ExFreePoolWithTag(HandleTable, TUN_MEMORY_TAG); - HandleTable = ExAllocatePoolUninitialized(PagedPool, RequestedSize, TUN_MEMORY_TAG); - if (!HandleTable) - return FALSE; - } - if (!NT_SUCCESS(Status) || !HandleTable) - goto cleanup; - - HANDLE CurrentProcessId = PsGetCurrentProcessId(); - for (ULONG_PTR Index = 0; Index < HandleTable->NumberOfHandles; ++Index) - { - FILE_OBJECT *FileObject = HandleTable->Handles[Index].Object; - if (!FileObject || FileObject->Type != 5 || FileObject->DeviceObject != DeviceObject) - continue; - HANDLE ProcessId = HandleTable->Handles[Index].UniqueProcessId; - if (ProcessId == CurrentProcessId) - continue; - Status = PsLookupProcessByProcessId(ProcessId, &Process); - if (!NT_SUCCESS(Status)) - continue; - KeStackAttachProcess(Process, &ApcState); - if (!VerifierFlags) - Status = ObReferenceObjectByHandle( - HandleTable->Handles[Index].HandleValue, 0, NULL, UserMode, &Object, &HandleInfo); - if (NT_SUCCESS(Status)) - { - if (VerifierFlags || Object == FileObject) - ObCloseHandle(HandleTable->Handles[Index].HandleValue, UserMode); - if (!VerifierFlags) - ObfDereferenceObject(Object); - DidClose = TRUE; - } - KeUnstackDetachProcess(&ApcState); - ObfDereferenceObject(Process); - } -cleanup: - if (HandleTable) - ExFreePoolWithTag(HandleTable, TUN_MEMORY_TAG); - return DidClose; -} - _IRQL_requires_max_(PASSIVE_LEVEL) static VOID TunProcessNotification(HANDLE ParentId, HANDLE ProcessId, BOOLEAN Create) @@ -876,8 +812,7 @@ static NTSTATUS TunDispatchDeviceControl(DEVICE_OBJECT *DeviceObject, IRP *Irp) { IO_STACK_LOCATION *Stack = IoGetCurrentIrpStackLocation(Irp); - if (Stack->Parameters.DeviceIoControl.IoControlCode != TUN_IOCTL_REGISTER_RINGS && - Stack->Parameters.DeviceIoControl.IoControlCode != TUN_IOCTL_FORCE_CLOSE_HANDLES) + if (Stack->Parameters.DeviceIoControl.IoControlCode != TUN_IOCTL_REGISTER_RINGS) return NdisDispatchDeviceControl(DeviceObject, Irp); SECURITY_SUBJECT_CONTEXT SubjectContext; @@ -912,9 +847,6 @@ TunDispatchDeviceControl(DEVICE_OBJECT *DeviceObject, IRP *Irp) KeLeaveCriticalRegion(); break; } - case TUN_IOCTL_FORCE_CLOSE_HANDLES: - Status = TunForceHandlesClosed(Stack->FileObject->DeviceObject) ? STATUS_SUCCESS : STATUS_NOTHING_TO_TERMINATE; - break; } cleanup: Irp->IoStatus.Status = Status; @@ -940,6 +872,79 @@ TunDispatchClose(DEVICE_OBJECT *DeviceObject, IRP *Irp) return NdisDispatchClose(DeviceObject, Irp); } +_Dispatch_type_(IRP_MJ_PNP) +static DRIVER_DISPATCH_PAGED DispatchPnp; +_Use_decl_annotations_ +static NTSTATUS +TunDispatchPnp(DEVICE_OBJECT *DeviceObject, IRP *Irp) +{ + IO_STACK_LOCATION *Stack = IoGetCurrentIrpStackLocation(Irp); + if (Stack->MinorFunction != IRP_MN_QUERY_REMOVE_DEVICE && Stack->MinorFunction != IRP_MN_SURPRISE_REMOVAL) + goto ndisDispatch; + + TUN_CTX *Ctx = DeviceObject->Reserved; + if (!Ctx) + goto ndisDispatch; + + ExAcquireResourceExclusiveLite(&Ctx->Device.RegistrationLock, TRUE); + if (!Ctx->Device.OwningFileObject || Ctx->Device.OwningFileObject == Stack->FileObject) + goto cleanupLock; + + NTSTATUS Status; + PEPROCESS Process; + KAPC_STATE ApcState; + PVOID Object = NULL; + OBJECT_HANDLE_INFORMATION HandleInfo; + SYSTEM_HANDLE_INFORMATION_EX *HandleTable = NULL; + ULONG VerifierFlags = 0; + + for (ULONG Size = 0, RequestedSize; + (Status = ZwQuerySystemInformation(SystemExtendedHandleInformation, HandleTable, Size, &RequestedSize)) == + STATUS_INFO_LENGTH_MISMATCH; + Size = RequestedSize) + { + if (HandleTable) + ExFreePoolWithTag(HandleTable, TUN_MEMORY_TAG); + HandleTable = ExAllocatePoolUninitialized(PagedPool, RequestedSize, TUN_MEMORY_TAG); + if (!HandleTable) + break; + } + if (!NT_SUCCESS(Status) || !HandleTable) + goto cleanupHandleTable; + + MmIsVerifierEnabled(&VerifierFlags); + + for (ULONG_PTR Index = 0; Index < HandleTable->NumberOfHandles; ++Index) + { + FILE_OBJECT *FileObject = HandleTable->Handles[Index].Object; + if (FileObject != Ctx->Device.OwningFileObject) + continue; + Status = PsLookupProcessByProcessId(HandleTable->Handles[Index].UniqueProcessId, &Process); + if (!NT_SUCCESS(Status)) + continue; + KeStackAttachProcess(Process, &ApcState); + if (!VerifierFlags) + Status = ObReferenceObjectByHandle( + HandleTable->Handles[Index].HandleValue, 0, NULL, UserMode, &Object, &HandleInfo); + if (NT_SUCCESS(Status)) + { + if (VerifierFlags || Object == FileObject) + ObCloseHandle(HandleTable->Handles[Index].HandleValue, UserMode); + if (!VerifierFlags) + ObfDereferenceObject(Object); + } + KeUnstackDetachProcess(&ApcState); + ObfDereferenceObject(Process); + } +cleanupHandleTable: + if (HandleTable) + ExFreePoolWithTag(HandleTable, TUN_MEMORY_TAG); +cleanupLock: + ExReleaseResourceLite(&Ctx->Device.RegistrationLock); +ndisDispatch: + return NdisDispatchPnp(DeviceObject, Irp); +} + static MINIPORT_RESTART TunRestart; _Use_decl_annotations_ static NDIS_STATUS @@ -1474,8 +1479,10 @@ DriverEntry(DRIVER_OBJECT *DriverObject, UNICODE_STRING *RegistryPath) NdisDispatchDeviceControl = DriverObject->MajorFunction[IRP_MJ_DEVICE_CONTROL]; NdisDispatchClose = DriverObject->MajorFunction[IRP_MJ_CLOSE]; + NdisDispatchPnp = DriverObject->MajorFunction[IRP_MJ_PNP]; DriverObject->MajorFunction[IRP_MJ_DEVICE_CONTROL] = TunDispatchDeviceControl; DriverObject->MajorFunction[IRP_MJ_CLOSE] = TunDispatchClose; + DriverObject->MajorFunction[IRP_MJ_PNP] = TunDispatchPnp; return STATUS_SUCCESS;