From 907bec05936296900b17ff38911dff3055ff8aea Mon Sep 17 00:00:00 2001
From: Alvin Ji <[email protected]>
Date: Mon, 04 May 2026 13:03:31 -0700
Subject: [PATCH] hid: Prevent UAF in HidConnectionWin on Close

This change adopts a self-owning pattern for aborted PendingHidTransfer
objects, ensuring that the OVERLAPPED structure and its associated
buffer stay valid until the Windows kernel finishes its asynchronous
read/write.

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

diff --git a/services/device/hid/hid_connection_win.cc b/services/device/hid/hid_connection_win.cc
index ba1a0262..e94b062 100644
--- a/services/device/hid/hid_connection_win.cc
+++ b/services/device/hid/hid_connection_win.cc
@@ -15,6 +15,7 @@
 #include "base/memory/advanced_memory_safety_checks.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/task/sequenced_task_runner.h"
 #include "base/win/object_watcher.h"
 #include "components/device_event_log/device_event_log.h"
 #include "services/device/public/cpp/device_features.h"
@@ -65,6 +66,20 @@
 
   void TakeResultFromWindowsAPI(BOOL result);
 
+  void Abort() {
+    if (base::FeatureList::IsEnabled(features::kSafeHidConnectionWinClose)) {
+      is_aborted_ = true;
+    } else {
+      watcher_.StopWatching();
+    }
+
+    if (callback_) {
+      std::move(callback_).Run(this, false);
+    }
+  }
+
+  bool is_aborted() const { return is_aborted_; }
+
   OVERLAPPED* GetOverlapped() { return &overlapped_; }
 
   // Implements base::win::ObjectWatcher::Delegate.
@@ -78,6 +93,7 @@
   OVERLAPPED overlapped_;
   base::win::ScopedHandle event_;
   base::win::ObjectWatcher watcher_;
+  bool is_aborted_ = false;
 };
 
 PendingHidTransfer::PendingHidTransfer(
@@ -107,6 +123,19 @@
 }
 
 void PendingHidTransfer::OnObjectSignaled(HANDLE event_handle) {
+  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 PendingHidTransfer.
+    base::SequencedTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE, this);
+    return;
+  }
+
+  // PendingHidTransfer 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 can have exclusive ownership of the buffer.
+  buffer_.reset();
   std::move(callback_).Run(this, true);
 }
 
@@ -144,7 +173,13 @@
     entry->file_handle.Close();
   }
   file_handles_.clear();
-  transfers_.clear();
+
+  // Abort() triggers the completion callback (e.g., OnReadInputReport),
+  // which calls UnlinkTransfer() and removes the entry from |transfers_|.
+  // We use a while loop to safely empty the list as entries are removed.
+  while (!transfers_.empty()) {
+    transfers_.front()->Abort();
+  }
 }
 
 void HidConnectionWin::PlatformWrite(
@@ -243,12 +278,18 @@
     scoped_refptr<base::RefCountedBytes> buffer,
     PendingHidTransfer* transfer_raw,
     bool signaled) {
+  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (transfer->is_aborted()) {
+    // `transfer` owns itself and will self-destruct when signaled by the OS.
+    transfer.release();
+    return;
+  }
+
   if (!signaled) {
     HID_LOG(DEBUG) << "HID read failed.";
     return;
   }
 
-  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
   if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
                            &bytes_transferred, FALSE)) {
@@ -278,13 +319,22 @@
     ReadCallback callback,
     PendingHidTransfer* transfer_raw,
     bool signaled) {
+  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (transfer->is_aborted()) {
+    // `transfer` owns itself and will self-destruct when signaled by the OS.
+    transfer.release();
+    // Handle the aborted case by signaling failure.
+    std::move(callback).Run(false, nullptr, 0);
+    return;
+  }
+
+  // Handle other erroneous cases.
   if (!signaled) {
     HID_LOG(DEBUG) << "HID read failed.";
     std::move(callback).Run(false, nullptr, 0);
     return;
   }
 
-  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
   if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
                            &bytes_transferred, FALSE)) {
@@ -310,13 +360,22 @@
                                        WriteCallback callback,
                                        PendingHidTransfer* transfer_raw,
                                        bool signaled) {
+  std::unique_ptr<PendingHidTransfer> transfer = UnlinkTransfer(transfer_raw);
+  if (transfer->is_aborted()) {
+    // `transfer` owns itself and will self-destruct when signaled by the OS.
+    transfer.release();
+    // Handle the aborted case by signaling failure.
+    std::move(callback).Run(false);
+    return;
+  }
+
+  // Handle other erroneous cases.
   if (!signaled) {
     HID_LOG(DEBUG) << "HID write failed.";
     std::move(callback).Run(false);
     return;
   }
 
-  auto transfer = UnlinkTransfer(transfer_raw);
   DWORD bytes_transferred;
   if (!GetOverlappedResult(file_handle, transfer->GetOverlapped(),
                            &bytes_transferred, FALSE)) {
diff --git a/services/device/public/cpp/device_features.cc b/services/device/public/cpp/device_features.cc
index 92bc1d0..f4baf7b 100644
--- a/services/device/public/cpp/device_features.cc
+++ b/services/device/public/cpp/device_features.cc
@@ -62,6 +62,11 @@
 // handle is closed.
 BASE_FEATURE(kSafeUsbDeviceHandleWinClose, base::FEATURE_ENABLED_BY_DEFAULT);
 
+// When enabled, HidConnectionWin will ensure that pending OVERLAPPED requests
+// are not deleted until the kernel has signaled completion, even if the
+// connection is closed.
+BASE_FEATURE(kSafeHidConnectionWinClose, 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 ecb77e18..b9cd547 100644
--- a/services/device/public/cpp/device_features.h
+++ b/services/device/public/cpp/device_features.h
@@ -34,6 +34,7 @@
     kWinSystemLocationPermissionEventBased);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kHidGetFeatureReportFix);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kSafeUsbDeviceHandleWinClose);
+DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kSafeHidConnectionWinClose);
 
 extern const DEVICE_FEATURES_EXPORT base::FeatureParam<int>
     kWinSystemLocationPermissionPollingParam;