From 67170c6099d8e3384de7e05617a7877c293df5a7 Mon Sep 17 00:00:00 2001
From: Alvin Ji <[email protected]>
Date: Fri, 24 Apr 2026 12:48:42 -0700
Subject: [PATCH] usb: Prevent UAF in UsbDeviceHandleWin on Close

Ensures pending WinUSB OVERLAPPED requests and their data buffers remain
alive until the kernel signals completion, even if the handle is closed.

- Introduces `kSafeUsbDeviceHandleWinClose` feature flag.
- Moves aborted requests to a global list until the OS signals completion.
- Ensures `Request` objects hold a reference to the transfer buffer to prevent memory corruption.

Change-Id: I8ac1dc4ca2a6dabb95c92ef80b9a46cb5bffb57f
Bug: 496233132
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7710634
Reviewed-by: Matt Reynolds <[email protected]>
Commit-Queue: Alvin Ji <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1620405}
---

diff --git a/services/device/public/cpp/device_features.cc b/services/device/public/cpp/device_features.cc
index c25a717..92bc1d0 100644
--- a/services/device/public/cpp/device_features.cc
+++ b/services/device/public/cpp/device_features.cc
@@ -57,6 +57,11 @@
 // start of the report and truncate the last byte of the report.
 BASE_FEATURE(kHidGetFeatureReportFix, base::FEATURE_ENABLED_BY_DEFAULT);
 
+// When enabled, UsbDeviceHandleWin will ensure that pending OVERLAPPED requests
+// are not deleted until the kernel has signaled completion, even if the
+// handle is closed.
+BASE_FEATURE(kSafeUsbDeviceHandleWinClose, base::FEATURE_ENABLED_BY_DEFAULT);
+
 // Defines a feature parameter for the `kWinSystemLocationPermission` feature.
 // This parameter controls the polling interval (in milliseconds) for checking
 // the permission status. The default polling interval is set to 500
diff --git a/services/device/public/cpp/device_features.h b/services/device/public/cpp/device_features.h
index e358bac..ecb77e18 100644
--- a/services/device/public/cpp/device_features.h
+++ b/services/device/public/cpp/device_features.h
@@ -33,6 +33,7 @@
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(
     kWinSystemLocationPermissionEventBased);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kHidGetFeatureReportFix);
+DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kSafeUsbDeviceHandleWinClose);
 
 extern const DEVICE_FEATURES_EXPORT base::FeatureParam<int>
     kWinSystemLocationPermissionPollingParam;
diff --git a/services/device/usb/usb_device_handle_win.cc b/services/device/usb/usb_device_handle_win.cc
index f3c3d7c..25928bf 100644
--- a/services/device/usb/usb_device_handle_win.cc
+++ b/services/device/usb/usb_device_handle_win.cc
@@ -30,6 +30,7 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/win/object_watcher.h"
 #include "components/device_event_log/device_event_log.h"
+#include "services/device/public/cpp/device_features.h"
 #include "services/device/public/cpp/usb/usb_utils.h"
 #include "services/device/usb/usb_context.h"
 #include "services/device/usb/usb_descriptors.h"
@@ -155,7 +156,9 @@
   void MaybeStartWatching(
       BOOL success,
       DWORD last_error,
+      scoped_refptr<base::RefCountedBytes> buffer,
       base::OnceCallback<void(Request*, DWORD, size_t)> callback) {
+    buffer_ = std::move(buffer);
     callback_ = std::move(callback);
     if (success) {
       OnObjectSignaled(event_.Get());
@@ -168,21 +171,45 @@
   }
 
   void Abort() {
+    if (base::FeatureList::IsEnabled(features::kSafeUsbDeviceHandleWinClose)) {
+      is_aborted_ = true;
+      if (callback_) {
+        std::move(callback_).Run(this, ERROR_REQUEST_ABORTED, 0);
+      }
+      return;
+    }
     watcher_.StopWatching();
     std::move(callback_).Run(this, ERROR_REQUEST_ABORTED, 0);
   }
 
   OVERLAPPED* overlapped() { return &overlapped_; }
   int interface_number() const { return interface_number_; }
+  bool is_aborted() const { return is_aborted_; }
 
   // base::win::ObjectWatcher::Delegate
   void OnObjectSignaled(HANDLE object) override {
     DCHECK_EQ(object, event_.Get());
+
+    if (is_aborted_) {
+      // `this` owns itself and will self-destruct now that the OS has signaled
+      // the event. This releases the buffer and OVERLAPPED structure held by
+      // this Request.
+      base::SequencedTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE,
+                                                                 this);
+      return;
+    }
+
     DWORD size;
     BOOL result =
         WinUsb_GetOverlappedResult(handle_, &overlapped_, &size, true);
     DWORD last_error = GetLastError();
 
+    // Request holds a reference to the buffer to ensure the kernel has a valid
+    // memory location during the overlapped operation. In the non-aborted case,
+    // we release this reference before running the callback so that the
+    // callback (which also holds a reference) can have exclusive ownership of
+    // the buffer.
+    buffer_.reset();
     if (result)
       std::move(callback_).Run(this, ERROR_SUCCESS, size);
     else
@@ -196,6 +223,12 @@
   base::win::ScopedHandle event_;
   base::win::ObjectWatcher watcher_;
   base::OnceCallback<void(Request*, DWORD, size_t)> callback_;
+  // This buffer is held to ensure that the memory stays alive until the kernel
+  // has signaled completion of the overlapped I/O operation. In the abort case
+  // the Request owns itself until completion, and this will be the only
+  // reference to the buffer as the transfer callback has already been invoked.
+  scoped_refptr<base::RefCountedBytes> buffer_;
+  bool is_aborted_ = false;
 };
 
 UsbDeviceHandleWin::Interface::Interface() = default;
@@ -575,7 +608,7 @@
   }
   DWORD last_error = GetLastError();
   request->MaybeStartWatching(
-      result, last_error,
+      result, last_error, buffer,
       base::BindOnce(&UsbDeviceHandleWin::TransferComplete,
                      weak_factory_.GetWeakPtr(), std::move(callback),
                      std::move(buffer)));
@@ -989,7 +1022,7 @@
       /*LengthTransferred=*/nullptr, control_request->overlapped());
   DWORD last_error = GetLastError();
   control_request->MaybeStartWatching(
-      result, last_error,
+      result, last_error, buffer,
       base::BindOnce(&UsbDeviceHandleWin::TransferComplete,
                      weak_factory_.GetWeakPtr(), std::move(callback), buffer));
 }
@@ -1112,6 +1145,11 @@
   ReleaseInterfaceReference(&it->second);
 
   std::move(callback).Run(status, std::move(buffer), bytes_transferred);
+
+  if (request->is_aborted()) {
+    // `request` owns itself and will self-destruct when signaled by the OS.
+    request.release();
+  }
 }
 
 void UsbDeviceHandleWin::ReportIsochronousError(
diff --git a/services/device/usb/usb_device_handle_win.h b/services/device/usb/usb_device_handle_win.h
index 278e407..3125d63 100644
--- a/services/device/usb/usb_device_handle_win.h
+++ b/services/device/usb/usb_device_handle_win.h
@@ -33,6 +33,8 @@
 // UsbDeviceHandle class provides basic I/O related functionalities.
 class UsbDeviceHandleWin : public UsbDeviceHandle {
  public:
+  class Request;
+
   UsbDeviceHandleWin(const UsbDeviceHandleWin&) = delete;
   UsbDeviceHandleWin& operator=(const UsbDeviceHandleWin&) = delete;
 
@@ -101,7 +103,6 @@
 
  private:
   struct Interface;
-  class Request;
 
   using OpenInterfaceCallback = base::OnceCallback<void(Interface*)>;