From ffb3d5c5df87bac72b393efdf27a67911fc9fcb9 Mon Sep 17 00:00:00 2001
From: Alvin Ji <alvinji@chromium.org>
Date: Thu, 13 Aug 2026 16:13:40 -0700
Subject: [PATCH] [M151] usb: Refine state change blocking and fix state machine leak

This CL combines two cherry-picks for WebUSB state change handling in M151:

1. CL 7927353 (commit 25a76a0415efe731f1c29318a66896d3396f104d):
   - Refined browser-side security checks in DeviceImpl to allow concurrent
     operations on unrelated interfaces during state changes.
   - Guarded non-control transfers and ClearHalt with endpoint readiness checks.

2. CL 8127656 (commit a585c93ef3453f08d13b4c849dbdf9ba04b5cfe9):
   - Tracked device state changes per DeviceImpl instance to clean up the
     shared UsbDevice state flag on destructor / tab closure.
   - Replaced ReportBadMessage with graceful error responses across WebUSB
     Mojo methods to prevent innocent renderer kills when concurrent
     connections access the same physical device.

(cherry picked from commits 25a76a0415efe731f1c29318a66896d3396f104d and a585c93ef3453f08d13b4c849dbdf9ba04b5cfe9)

TAG=agy
CONV=9b304f49-900d-4515-b6d6-b0b58732fde6

Bug: 544750033, 535841139, 522732244
Change-Id: Icc402b968a1561abe33274c30def695207eb1870
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8228933
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/branch-heads/7922@{#3046}
Cr-Branched-From: 059c884787b1bc15faa5afe5250f7d3a7e0ef64a-refs/heads/main@{#1654411}
---

diff --git a/services/device/usb/mojo/device_impl.cc b/services/device/usb/mojo/device_impl.cc
index d1953e39..8e9918e 100644
--- a/services/device/usb/mojo/device_impl.cc
+++ b/services/device/usb/mojo/device_impl.cc
@@ -20,6 +20,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/notreached.h"
 #include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "services/device/public/cpp/device_features.h"
@@ -169,6 +170,12 @@
 }
 
 DeviceImpl::~DeviceImpl() {
+  if (device_state_change_in_progress_) {
+    device_state_change_in_progress_ = false;
+    if (device_) {
+      device_->set_device_state_change_in_progress(false);
+    }
+  }
   CloseHandle();
 }
 
@@ -453,8 +460,9 @@
 
 void DeviceImpl::SetConfiguration(uint8_t value,
                                   SetConfigurationCallback callback) {
-  if (device_->state_change_in_progress()) {
-    mojo::ReportBadMessage("Device state change in progress.");
+  if (any_state_change_in_progress()) {
+    RecordStateChangeBlocked(WebUsbStateChangeBlockedMethod::kSetConfiguration,
+                             any_interface_state_change_in_progress());
     std::move(callback).Run(false);
     return;
   }
@@ -463,7 +471,8 @@
     return;
   }
 
-  device_->set_state_change_in_progress(true);
+  device_state_change_in_progress_ = true;
+  device_->set_device_state_change_in_progress(true);
   device_handle_->SetConfiguration(
       value, base::BindOnce(&DeviceImpl::OnSetConfigurationComplete,
                             weak_factory_.GetWeakPtr(), std::move(callback)));
@@ -471,8 +480,11 @@
 
 void DeviceImpl::ClaimInterface(uint8_t interface_number,
                                 ClaimInterfaceCallback callback) {
-  if (device_->state_change_in_progress()) {
-    mojo::ReportBadMessage("Device state change in progress.");
+  bool interface_in_progress =
+      IsInterfaceStateChangeInProgress(interface_number);
+  if (device_->device_state_change_in_progress() || interface_in_progress) {
+    RecordStateChangeBlocked(WebUsbStateChangeBlockedMethod::kClaimInterface,
+                             interface_in_progress);
     std::move(callback).Run(mojom::UsbClaimInterfaceResult::kFailure);
     return;
   }
@@ -502,17 +514,20 @@
     }
   }
 
-  device_->set_state_change_in_progress(true);
+  SetInterfaceStateChangeInProgress(interface_number, true);
   device_handle_->ClaimInterface(
-      interface_number,
-      base::BindOnce(&DeviceImpl::OnInterfaceClaimed,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+      interface_number, base::BindOnce(&DeviceImpl::OnInterfaceClaimed,
+                                       weak_factory_.GetWeakPtr(),
+                                       std::move(callback), interface_number));
 }
 
 void DeviceImpl::ReleaseInterface(uint8_t interface_number,
                                   ReleaseInterfaceCallback callback) {
-  if (device_->state_change_in_progress()) {
-    mojo::ReportBadMessage("Device state change in progress.");
+  bool interface_in_progress =
+      IsInterfaceStateChangeInProgress(interface_number);
+  if (device_->device_state_change_in_progress() || interface_in_progress) {
+    RecordStateChangeBlocked(WebUsbStateChangeBlockedMethod::kReleaseInterface,
+                             interface_in_progress);
     std::move(callback).Run(false);
     return;
   }
@@ -521,15 +536,23 @@
     return;
   }
 
-  device_handle_->ReleaseInterface(interface_number, std::move(callback));
+  SetInterfaceStateChangeInProgress(interface_number, true);
+  device_handle_->ReleaseInterface(
+      interface_number, base::BindOnce(&DeviceImpl::OnInterfaceReleased,
+                                       weak_factory_.GetWeakPtr(),
+                                       std::move(callback), interface_number));
 }
 
 void DeviceImpl::SetInterfaceAlternateSetting(
     uint8_t interface_number,
     uint8_t alternate_setting,
     SetInterfaceAlternateSettingCallback callback) {
-  if (device_->state_change_in_progress()) {
-    mojo::ReportBadMessage("Device state change in progress.");
+  bool interface_in_progress =
+      IsInterfaceStateChangeInProgress(interface_number);
+  if (device_->device_state_change_in_progress() || interface_in_progress) {
+    RecordStateChangeBlocked(
+        WebUsbStateChangeBlockedMethod::kSetInterfaceAlternateSetting,
+        interface_in_progress);
     std::move(callback).Run(false);
     return;
   }
@@ -556,16 +579,18 @@
     return;
   }
 
-  device_->set_state_change_in_progress(true);
+  SetInterfaceStateChangeInProgress(interface_number, true);
   device_handle_->SetInterfaceAlternateSetting(
       interface_number, alternate_setting,
       base::BindOnce(&DeviceImpl::OnSetInterfaceAlternateSettingComplete,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+                     weak_factory_.GetWeakPtr(), std::move(callback),
+                     interface_number));
 }
 
 void DeviceImpl::Reset(ResetCallback callback) {
-  if (device_->state_change_in_progress()) {
-    mojo::ReportBadMessage("Device state change in progress.");
+  if (any_state_change_in_progress()) {
+    RecordStateChangeBlocked(WebUsbStateChangeBlockedMethod::kReset,
+                             any_interface_state_change_in_progress());
     std::move(callback).Run(false);
     return;
   }
@@ -574,7 +599,8 @@
     return;
   }
 
-  device_->set_state_change_in_progress(true);
+  device_state_change_in_progress_ = true;
+  device_->set_device_state_change_in_progress(true);
   device_handle_->ResetDevice(base::BindOnce(&DeviceImpl::OnResetComplete,
                                              weak_factory_.GetWeakPtr(),
                                              std::move(callback)));
@@ -583,7 +609,12 @@
 void DeviceImpl::ClearHalt(UsbTransferDirection direction,
                            uint8_t endpoint_number,
                            ClearHaltCallback callback) {
-  if (!device_handle_) {
+  uint8_t endpoint_address = endpoint_number;
+  if (direction == UsbTransferDirection::INBOUND) {
+    endpoint_address |= 0x80;
+  }
+  if (!IsEndpointReadyForTransfer(WebUsbStateChangeBlockedMethod::kClearHalt,
+                                  endpoint_address)) {
     std::move(callback).Run(false);
     return;
   }
@@ -595,6 +626,12 @@
                                    uint32_t length,
                                    uint32_t timeout,
                                    ControlTransferInCallback callback) {

diff --git a/services/device/usb/mojo/device_impl_unittest.cc b/services/device/usb/mojo/device_impl_unittest.cc
index 7df57bd..0a702533 100644
--- a/services/device/usb/mojo/device_impl_unittest.cc
+++ b/services/device/usb/mojo/device_impl_unittest.cc
@@ -82,6 +82,27 @@
     return *this;
   }
 
+  ConfigBuilder& AddEndpoint(uint8_t interface_number,
+                             uint8_t alternate_setting,
+                             uint8_t endpoint_address,
+                             uint8_t attributes,
+                             uint16_t maximum_packet_size,
+                             uint8_t polling_interval) {
+    for (auto& interface : config_->interfaces) {
+      if (interface->interface_number == interface_number) {
+        for (auto& alternate : interface->alternates) {
+          if (alternate->alternate_setting == alternate_setting) {
+            alternate->endpoints.push_back(
+                BuildUsbEndpointInfoPtr(endpoint_address, attributes,
+                                        maximum_packet_size, polling_interval));
+            return *this;
+          }
+        }
+      }
+    }
+    return *this;
+  }
+
   mojom::UsbConfigurationInfoPtr Build() { return std::move(config_); }
 
  private:
@@ -185,6 +206,15 @@
   bool is_device_open() const { return open_count_ > 0; }
   MockUsbDeviceHandle& mock_handle() { return *mock_handle_.get(); }
 
+  void WaitForDeviceClose() {
+    if (!is_device_open()) {
+      return;
+    }
+    base::RunLoop run_loop;
+    device_close_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
   void set_allow_reset(bool allow_reset) { allow_reset_ = allow_reset; }
 
   // Creates a mock device and binds a Device proxy to a Device service impl
@@ -233,6 +263,9 @@
     ON_CALL(mock_handle(), IsochronousTransferOutInternal(_, _, _, _, _))
         .WillByDefault(
             Invoke(this, &USBDeviceImplTest::IsochronousTransferOut));
+    ON_CALL(mock_handle(), FindInterfaceByEndpoint(_))
+        .WillByDefault(
+            Invoke(this, &USBDeviceImplTest::FindInterfaceByEndpoint));
 
     return proxy;
   }
@@ -264,6 +297,15 @@
     return GetMockDeviceProxy(/*client=*/mojo::NullRemote());
   }
 
+  mojo::Remote<mojom::UsbDevice> GetNewMockDeviceProxyForExistingDevice() {
+    mojo::Remote<mojom::UsbDevice> proxy;
+    DeviceImpl::Create(mock_device_, proxy.BindNewPipeAndPassReceiver(),
+                       mojo::NullRemote(),
+                       /*blocked_interface_classes=*/{},
+                       /*allow_security_key_requests=*/false);
+    return proxy;
+  }
+
   void AddMockConfig(mojom::UsbConfigurationInfoPtr config) {
     DCHECK(!mock_configs_.contains(config->configuration_value));
     mock_configs_.insert(
@@ -291,6 +333,25 @@
     mock_outbound_packets_.push(std::move(packets));
   }
 
+  void ConfigureAndClaimInterface(mojom::UsbDevice* device,
+                                  uint8_t configuration_value,
+                                  uint8_t interface_number) {
+    EXPECT_CALL(mock_handle(),
+                SetConfigurationInternal(configuration_value, _));
+    base::test::TestFuture<bool> set_config_future;
+    device->SetConfiguration(configuration_value,
+                             set_config_future.GetCallback());
+    EXPECT_TRUE(set_config_future.Get());
+
+    EXPECT_CALL(mock_handle(), ClaimInterfaceInternal(interface_number, _));
+    base::test::TestFuture<mojom::UsbClaimInterfaceResult>
+        claim_interface_future;
+    device->ClaimInterface(interface_number,
+                           claim_interface_future.GetCallback());
+    EXPECT_EQ(claim_interface_future.Get(),
+              mojom::UsbClaimInterfaceResult::kSuccess);
+  }
+
  private:
   void OpenMockHandle(UsbDevice::OpenCallback& callback) {
     open_count_++;
@@ -303,6 +364,9 @@
   void CloseMockHandle() {
     EXPECT_GT(open_count_, 0);
     open_count_--;
+    if (open_count_ == 0 && device_close_closure_) {
+      std::move(device_close_closure_).Run();
+    }
   }
 
   void SetConfiguration(uint8_t value,
@@ -339,6 +403,34 @@
     }
   }
 
+  const mojom::UsbInterfaceInfo* FindInterfaceByEndpoint(
+      uint8_t endpoint_address) {
+    const mojom::UsbConfigurationInfo* config =
+        mock_device().GetActiveConfiguration();
+    if (!config) {
+      return nullptr;
+    }
+
+    for (const auto& interface : config->interfaces) {
+      if (!claimed_interfaces_.contains(interface->interface_number)) {
+        continue;
+      }
+
+      for (const auto& alternate : interface->alternates) {
+        for (const auto& endpoint : alternate->endpoints) {
+          uint8_t address = endpoint->endpoint_number;
+          if (endpoint->direction == mojom::UsbTransferDirection::INBOUND) {
+            address |= 0x80;
+          }
+          if (address == endpoint_address) {
+            return interface.get();
+          }
+        }
+      }
+    }
+    return nullptr;
+  }
+
   void SetInterfaceAlternateSetting(uint8_t interface_number,
                                     uint8_t alternate_setting,
                                     UsbDeviceHandle::ResultCallback& callback) {
@@ -478,6 +570,7 @@
   base::queue<std::vector<UsbIsochronousPacketPtr>> mock_outbound_packets_;
 
   std::set<uint8_t> claimed_interfaces_;
+  base::OnceClosure device_close_closure_;
 };
 
 }  // namespace
@@ -907,6 +1000,557 @@
   EXPECT_CALL(mock_handle(), Close());
 }
 
+// Verify that ControlTransferIn requests are blocked and rejected as a bad
+// message if a device configuration change is currently in progress.
+TEST_F(USBDeviceImplTest, ControlTransferInBlockedDuringSetConfiguration) {
+  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  base::test::TestFuture<mojom::UsbOpenDeviceResultPtr> open_future;
+  device->Open(open_future.GetCallback());
+  EXPECT_TRUE(open_future.Get()->is_success());
+
+  AddMockConfig(ConfigBuilder(/*configuration_value=*/1)
+                    .AddInterface(/*interface_number=*/0,
+                                  /*alternate_setting=*/0,
+                                  /*class_code=*/1,
+                                  /*subclass_code=*/2,
+                                  /*protocol_code=*/3)
+                    .Build());
+
+  base::test::TestFuture<UsbDeviceHandle::ResultCallback>
+      set_configuration_future;
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _))
+      .WillOnce([&](int value, UsbDeviceHandle::ResultCallback& callback) {
+        set_configuration_future.SetValue(std::move(callback));
+      });
+
+  // Initiate SetConfiguration but only save the callback without invoking it.
+  device->SetConfiguration(1, base::NullCallback());
+
+  // Ensure the request has reached the service.
+  ASSERT_TRUE(set_configuration_future.Wait());
+
+  // Immediately try to do a control transfer; should fail as a bad message.
+  auto params = mojom::UsbControlTransferParams::New();
+  params->type = mojom::UsbControlTransferType::STANDARD;
+  params->recipient = mojom::UsbControlTransferRecipient::DEVICE;
+  params->request = 0;
+  params->value = 0;
+  params->index = 0;
+
+  base::test::TestFuture<mojom::UsbTransferStatus, base::span<const uint8_t>>
+      transfer_future;
+  device->ControlTransferIn(std::move(params), 8, 0,
+                            transfer_future.GetCallback());
+  EXPECT_EQ(transfer_future.Get<0>(),
+            mojom::UsbTransferStatus::PERMISSION_DENIED);
+
+  device.reset();
+  WaitForDeviceClose();
+}
+
+// Verify that ControlTransferOut requests are blocked and rejected as a bad
+// message if a device configuration change is currently in progress.
+TEST_F(USBDeviceImplTest, ControlTransferOutBlockedDuringSetConfiguration) {
+  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  base::test::TestFuture<mojom::UsbOpenDeviceResultPtr> open_future;
+  device->Open(open_future.GetCallback());
+  EXPECT_TRUE(open_future.Get()->is_success());
+
+  AddMockConfig(ConfigBuilder(/*configuration_value=*/1)
+                    .AddInterface(/*interface_number=*/0,
+                                  /*alternate_setting=*/0,
+                                  /*class_code=*/1,
+                                  /*subclass_code=*/2,
+                                  /*protocol_code=*/3)
+                    .Build());
+
+  base::test::TestFuture<UsbDeviceHandle::ResultCallback>
+      set_configuration_future;
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _))
+      .WillOnce([&](int value, UsbDeviceHandle::ResultCallback& callback) {
+        set_configuration_future.SetValue(std::move(callback));
+      });
+
+  // Initiate SetConfiguration but only save the callback without invoking it.
+  device->SetConfiguration(1, base::NullCallback());
+
+  // Ensure the request has reached the service.
+  ASSERT_TRUE(set_configuration_future.Wait());
+
+  // Immediately try to do a control transfer; should fail as a bad message.
+  auto params = mojom::UsbControlTransferParams::New();
+  params->type = mojom::UsbControlTransferType::STANDARD;
+  params->recipient = mojom::UsbControlTransferRecipient::DEVICE;
+  params->request = 0;
+  params->value = 0;
+  params->index = 0;
+
+  base::test::TestFuture<mojom::UsbTransferStatus> transfer_future;
+  std::vector<uint8_t> data = {1, 2, 3, 4};
+  device->ControlTransferOut(std::move(params), data, 0,
+                             transfer_future.GetCallback());
+  EXPECT_EQ(transfer_future.Get(), mojom::UsbTransferStatus::PERMISSION_DENIED);
+
+  device.reset();
+  WaitForDeviceClose();
+}
+
+TEST_F(USBDeviceImplTest, ControlTransferInBlockedDuringReleaseInterface) {
+  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  base::test::TestFuture<mojom::UsbOpenDeviceResultPtr> open_future;
+  device->Open(open_future.GetCallback());
+  EXPECT_TRUE(open_future.Get()->is_success());
+
+  AddMockConfig(ConfigBuilder(/*configuration_value=*/1)
+                    .AddInterface(/*interface_number=*/1,
+                                  /*alternate_setting=*/0,
+                                  /*class_code=*/1,
+                                  /*subclass_code=*/2,
+                                  /*protocol_code=*/3)
+                    .Build());
+
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _));
+
+  base::test::TestFuture<bool> set_config_future;
+  device->SetConfiguration(1, set_config_future.GetCallback());
+  EXPECT_TRUE(set_config_future.Get());
+
+  EXPECT_CALL(mock_handle(), ClaimInterfaceInternal(1, _));
+
+  base::test::TestFuture<mojom::UsbClaimInterfaceResult> claim_interface_future;
+  device->ClaimInterface(1, claim_interface_future.GetCallback());
+  EXPECT_EQ(claim_interface_future.Get(),
+            mojom::UsbClaimInterfaceResult::kSuccess);
+
+  base::test::TestFuture<int, UsbDeviceHandle::ResultCallback>
+      release_interface_future;
+  EXPECT_CALL(mock_handle(), ReleaseInterfaceInternal(1, _))
+      .WillOnce([&](int value, UsbDeviceHandle::ResultCallback& callback) {
+        release_interface_future.SetValue(value, std::move(callback));
+      });
+
+  // Initiate ReleaseInterface but only save the callback without invoking it.
+  device->ReleaseInterface(1, base::NullCallback());
+
+  // Ensure the request has reached the service.
+  ASSERT_TRUE(release_interface_future.Wait());
... (truncated)