Mcaster1AudioPipe — Security Audit Report

The stream destructor frees the DMA buffer, scratch buffer, and releases COM references. However, KeCancelTimer() only dequeues pending timers — it does not wait for an already-executing DPC to complete. If the timer DPC was mid-execution when the destructor ran, ProcessAudioPump() would access freed memory, causing a kernel use-after-free (BSOD).

Impact: Kernel-mode use-after-free leading to BSOD (IRQL_NOT_LESS_OR_EQUAL or PAGE_FAULT_IN_NONPAGED_AREA).

 CMiniportWaveRTStream::~CMiniportWaveRTStream()
 {
-    KeCancelTimer(&m_Timer);
-    // DPC may still be executing here! UAF follows.
+    if (InterlockedCompareExchange(&m_TimerActive, 0, 1) == 1) {
+        KeCancelTimer(&m_Timer);
+        KeFlushQueuedDpcs();  // Wait for in-flight DPC to complete
+    }
     if (m_ScratchBuffer) {
         ExFreePoolWithTag(m_ScratchBuffer, MCASTER1_POOL_TAG);

Fix: Added KeFlushQueuedDpcs() after KeCancelTimer() to ensure any in-flight DPC completes before freeing resources. Guarded by InterlockedCompareExchange on the m_TimerActive flag.

The SetState(KSSTATE_STOP) and SetState(KSSTATE_PAUSE) transitions cancelled the timer but did not wait for an in-flight DPC. A concurrent DPC execution could access the DMA buffer while it was being zeroed or after position was reset.

 case KSSTATE_STOP:
-    KeCancelTimer(&m_Timer);
+    if (InterlockedCompareExchange(&m_TimerActive, 0, 1) == 1) {
+        KeCancelTimer(&m_Timer);
+        KeFlushQueuedDpcs();
+    }
     m_LinearPosition = 0;
 ...
 case KSSTATE_PAUSE:
     if (m_State == KSSTATE_RUN) {
-        KeCancelTimer(&m_Timer);
+        if (InterlockedCompareExchange(&m_TimerActive, 0, 1) == 1) {
+            KeCancelTimer(&m_Timer);
+            KeFlushQueuedDpcs();
+        }
     }

Fix: Applied the same KeFlushQueuedDpcs() pattern to all state transitions that cancel the timer. Matches the destructor fix (KD-C1).

CalculateFramesToProcess() is called from the timer DPC at DISPATCH_LEVEL. If m_PipeDevice was NULL (e.g., during teardown) or perfFrequency was zero, the function would dereference a NULL pointer or divide by zero, causing BSOD.

 ULONG CMiniportWaveRTStream::CalculateFramesToProcess()
 {
+    if (!m_PipeDevice) return 0;
+
     LONGLONG perfFreq = m_PipeDevice->perfFrequency.QuadPart;
+    if (perfFreq == 0) return 0;  /* Prevent division by zero */

Fix: Added NULL guard for m_PipeDevice and division-by-zero guard for perfFreq. Same guards applied to GetPosition().

g_PhysicalDeviceObject is a global pointer written in Mcaster1AddDevice() and read in Mcaster1StartDevice(). If multiple instances were added concurrently, the pointer could be overwritten before the first StartDevice completes.

 static PDEVICE_OBJECT g_PhysicalDeviceObject = NULL;
 ...
 static NTSTATUS Mcaster1AddDevice(...) {
     g_PhysicalDeviceObject = PhysicalDeviceObject;

Mitigation: ROOT-enumerated PnP devices are serialized by the PnP manager — AddDevice and StartDevice are called sequentially for each device stack. Multi-instance creation (via devcon) is serialized at the PnP manager level. Risk is theoretical only for this driver class. Documented as acceptable.

SetCustomName() copies a wide string into dev->name[128]. An unbounded wcscpy would overflow the buffer if the source exceeded 127 characters.

-    wcscpy(dev->name, name);
+    RtlStringCchCopyW(dev->name, ARRAYSIZE(dev->name), name);

Fix: Replaced wcscpy with the safe RtlStringCchCopyW which truncates at the array boundary and NUL-terminates.

ClaimNextDeviceSlot() uses InterlockedCompareExchange(&active, 1, 0) to atomically claim a slot. However, the slot fields (sampleRate, channels, ringBuffer, etc.) were initialized after active was set to 1, creating a window where GetPipeDevice() could return a partially-initialized device.

 if (InterlockedCompareExchange(&g_Devices[i].active, 1, 0) == 0) {
     g_Devices[i].slot = i;
     g_Devices[i].sampleRate = sampleRate;
     ... /* all field initialization */
+    MemoryBarrier();  /* Ensure all writes visible before reads */
     return i;

Fix: Added MemoryBarrier() after all field initialization to ensure a full fence before the slot is read by other threads. The active flag was already set atomically; the barrier ensures field writes are not reordered past the return.

If SetState(STOP) was called multiple times (e.g., during error recovery), InterlockedDecrement(&isRunning) would underflow past zero. Subsequent isRunning checks would see a negative value and behave incorrectly.

-    if (m_PipeDevice) {
-        InterlockedDecrement(&m_PipeDevice->isRunning);
-    }
+    if (m_PipeDevice && m_WasRunning) {
+        m_WasRunning = FALSE;
+        if (InterlockedDecrement(&m_PipeDevice->isRunning) <= 0) {
+            InterlockedExchange(&m_PipeDevice->isRunning, 0);
+            InterlockedExchange(&m_PipeDevice->clockRunning, 0);
+        }
+    }

Fix: Added m_WasRunning per-stream tracking. Only decrements if this stream actually set isRunning during RUN transition. Clamps to zero on underflow.

WriteDiagJackCount() called ZwCreateKey/ZwSetValueKey (pageable APIs) from inside PropertyHandler_JackDescription(). PortCls may invoke property handlers at DISPATCH_LEVEL, where pageable code causes IRQL_NOT_LESS_OR_EQUAL BSOD.

 InterlockedIncrement(&g_JackQueryCount);
-    WriteDiagJackCount();  // ZwCreateKey at DISPATCH_LEVEL = BSOD
+    /* WriteDiagJackCount removed -- ZwCreateKey/ZwSetValueKey are pageable APIs
+     * and MUST NOT be called at DISPATCH_LEVEL. Jack query count is still
+     * tracked in memory via g_JackQueryCount. */

Fix: Removed the registry write from the DPC-callable property handler. The counter is still tracked in-memory via InterlockedIncrement and can be read via a dedicated IOCTL at PASSIVE_LEVEL.

Mcaster1StartDevice() allocates WCHAR customName[128] (256 bytes) and UCHAR buf[sizeof(KEY_VALUE_PARTIAL_INFORMATION) + 256 * sizeof(WCHAR)] (~524 bytes) on the kernel stack. The kernel stack is only 12–24KB, so large allocations risk overflow.

Mitigation: StartDevice runs at PASSIVE_LEVEL with a full-size kernel stack (~24KB on x64). The 524-byte allocation uses ~2% of available stack and is well within safe limits. No remediation required — this is standard practice per MSVAD sample code.

After KD-H5 removed the call to WriteDiagJackCount(), the function body became dead code. Dead code increases attack surface and complicates auditing.

Fix: The call was removed and the function retained as a diagnostic utility callable from a future PASSIVE_LEVEL IOCTL handler. Marked with a comment explaining it is intentionally unused in the hot path.

The ring buffer uses InterlockedExchange64 for write_pos/read_pos updates, which provides a full barrier on x86_64 but may have weaker guarantees on ARM64. The producer writes audio data via RtlCopyMemory and then updates write_pos — on ARM64, the copy could be reordered past the position update without explicit acquire/release semantics.

Mitigation: InterlockedExchange64 on Windows ARM64 does use full barriers (the Interlocked* family on Windows always provides sequential consistency). However, the RtlCopyMemory before the exchange may need an explicit MemoryBarrier() on ARM64 to prevent store reordering. Currently x64-only; will be addressed when ARM64 support ships.

All 10 IOCTL codes use FILE_ANY_ACCESS, meaning any process that can open the device handle can invoke any IOCTL (create pipe, destroy pipe, set volume, etc.). Write operations should use FILE_WRITE_ACCESS to require write permissions on the device handle.

 #define IOCTL_MCASTER1_CREATE_PIPE \
     CTL_CODE(MCASTER1_DEVICE_TYPE, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS)
 /* Should be FILE_WRITE_ACCESS for mutating operations */

Mitigation: IOCTLs are not directly exposed to userspace in the current architecture (no device symlink). The IPC server mediates all access via Named Pipes with its own ACLs. Will be hardened when the driver exposes a direct user-mode control channel.

underrunCount and overrunCount are volatile LONGLONG (64-bit) incremented from the DPC. On 32-bit x86, a 64-bit increment is not atomic and could produce torn values.

Fix: Changed to InterlockedIncrement64() which guarantees atomic 64-bit increment on all architectures.

GetDeviceDescription originally set MaximumLength = 0xFFFFFFFF, allowing PortCls to allocate up to 4GB for a single DMA transfer. For a virtual device with 10ms timer periods, this is wildly excessive.

Fix: Capped at 0x100000 (1MB) — sufficient for 1 second of 96kHz stereo Float32 at the maximum buffer size.

The StartDevice cleanup path previously missed releasing pPortTopology if wave port creation failed after topology was already initialized. This leaked a PortCls port object.

Fix: All six COM pointers are unconditionally released in the cleanup label (NULL-safe: calling Release on NULL is checked with an if-guard).

GetPosition() originally called KeQueryPerformanceCounter(NULL) to get both the timestamp and the frequency on every call. The frequency is constant and should be cached. Double QPC calls waste cycles in a latency-sensitive path.

Fix: Cached perfFrequency in the PIPE_DEVICE_WIN struct (set once during ClaimNextDeviceSlot). Both GetPosition() and CalculateFramesToProcess() now read the cached value.

populateHALDrivers() calls CoInitializeEx() and CoCreateInstance() for WASAPI device enumeration. If the function returned early (e.g., CoCreateInstance failed), CoUninitialize() was not called, leaking the COM apartment initialization. Repeated calls would accumulate COM references, eventually exhausting resources.

-    CoInitializeEx(nullptr, COINIT_MULTITHREADED);
-    // ... early returns without CoUninitialize ...
+    HRESULT comHr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
+    bool comNeedUninit = (comHr == S_OK || comHr == S_FALSE);
     // ... enumeration logic ...
+    if (comNeedUninit) CoUninitialize();

Fix: Track whether CoInitializeEx succeeded (S_OK or S_FALSE) and always call CoUninitialize at function end. Also fixed the threading model (see APP-H3).

The activity monitor widget called CoInitializeEx() in its meter initialization but did not track whether COM was initialized, leading to unbalanced CoInit/CoUninit calls when the widget was destroyed and recreated.

Fix: Added m_comInitialized member variable to track COM state. CoUninitialize is called in the destructor only if the matching CoInitializeEx succeeded.

onInstallDriver() runs QProcess::waitForFinished(60000) on the GUI thread, which blocks the entire UI for up to 60 seconds during driver installation. The UAC elevation prompt alone can take indefinite time if the user delays.

Fix: Replaced blocking waits with QProgressDialog + polling loop using QApplication::processEvents(). The progress dialog shows "Installing..." with a Cancel button and processes UI events during the wait.

The "Restart Audio Service" handler used QThread::msleep(2000) to wait for the service to restart, completely freezing the GUI for 2 seconds.

Fix: Replaced with a processEvents polling loop that checks service status every 200ms with a 5-second timeout, keeping the UI responsive.

Qt6 uses STA (Single-Threaded Apartment) internally for Windows event processing. Calling CoInitializeEx(nullptr, COINIT_MULTITHREADED) from the GUI thread creates a conflict: COM objects created in an MTA can receive calls from any thread, but Qt widgets are not thread-safe.

Fix: Changed to COINIT_APARTMENTTHREADED to match Qt's STA model.

DeviceEnumerator manages PortAudio initialization via static state. If multiple enumerator instances exist or if refresh() is called from a non-GUI thread, PortAudio's global state could be corrupted.

Mitigation: Only one DeviceEnumerator instance exists (owned by MainWindow). Refresh is only called from the GUI thread. Needs verification that no secondary instances are created during Patch Bay rebuild. Low probability in current architecture.

isOurDriverInstalled() runs pnputil /enum-drivers synchronously with a 10-second timeout, blocking the GUI thread during startup and refresh.

Mitigation: Acceptable startup cost (~200ms typical). The function is only called during explicit refresh() and initial load. Moving to async would complicate the state machine significantly for minimal UX benefit.

Driver install/uninstall scripts were written to a predictable temp file path, then executed. A local attacker could replace the file between creation and execution.

Fix: Replaced with QTemporaryFile which creates files with unique random names and exclusive access. Additionally, elevated commands are now passed inline via PowerShell -Command parameter where possible, eliminating the temp file entirely.

The app reads %SystemRoot% to locate system executables (powershell.exe, pnputil.exe, bcdedit.exe). A malicious user could set SystemRoot to a directory containing trojaned executables.

 QString systemRoot = QString::fromLocal8Bit(qgetenv("SystemRoot"));
 if (systemRoot.isEmpty()) systemRoot = "C:\\Windows";
+ /* Validation: verify kernel32.dll exists at the resolved path
+  * to confirm SystemRoot points to a real Windows installation */
 QString psPath = systemRoot + "\\System32\\WindowsPowerShell\\v1.0\\powershell.exe";

Fix: Added validation that kernel32.dll exists at the resolved SystemRoot path before trusting it. Falls back to C:\Windows if validation fails.

controlService() accepted empty label and action strings, which could result in passing empty arguments to net.exe via elevated PowerShell, potentially causing unexpected behavior.

Fix: Added early-return guard for empty label/action parameters before constructing the command string.

The uninstall path searches for the OEM INF by matching "Mcaster1AudioPipe" in the pnputil output, then extracts the oem*.inf name. If multiple driver versions are in the store, it may delete the wrong one.

Mitigation: The pnputil output matching is specific enough for single-version installs. Multi-version scenario is unlikely in normal use. Will be improved by parsing the full driver store entry.

The driverInstalled() and driverUninstalled() signals from AudioDriverManager trigger a Patch Bay rebuild. If the signal fires during initial construction (before patchBayWidget_ is initialized), the rebuild accesses an uninitialized pointer.

Mitigation: Construction order in MainWindow ensures patchBayWidget_ is created before AudioDriverManager connects signals. The init order is controlled by the constructor body, not by signal delivery timing.

Property dialogs for audio devices used a separate COM initialization that conflicted with the already-initialized STA apartment on the GUI thread.

Fix: Reuses the existing COM apartment initialized in populateHALDrivers() rather than creating a new one.