From 7e97805c0fd550b2a9b7c3e1012e6680e6b72011 Mon Sep 17 00:00:00 2001
From: Alvin Ji <alvinji@chromium.org>
Date: Mon, 11 May 2026 16:14:09 -0700
Subject: [PATCH] usb: Lock interface requests during pending device state changes

Fixes a TOCTOU vulnerability where a compromised renderer could pipeline
a ClaimInterface request immediately behind SetConfiguration, bypassing
the protected interface blocklist by validating against stale
configuration metadata.

Introduces a `device_state_change_in_progress_` flag to DeviceImpl to
synchronously reject interface operations while a device configuration
switch or reset is pending.

Bug: 499162550
Change-Id: Id7bbe4d655598814cdc0f8e03a92423360805732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7831277
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1628883}
---

diff --git a/services/device/usb/mojo/device_impl.cc b/services/device/usb/mojo/device_impl.cc
index 7f34bfb..db7f9c64 100644
--- a/services/device/usb/mojo/device_impl.cc
+++ b/services/device/usb/mojo/device_impl.cc
@@ -346,16 +346,29 @@
 
 void DeviceImpl::SetConfiguration(uint8_t value,
                                   SetConfigurationCallback callback) {
+  if (device_state_change_in_progress_) {
+    mojo::ReportBadMessage("Device state change in progress.");
+    std::move(callback).Run(false);
+    return;
+  }
   if (!device_handle_) {
     std::move(callback).Run(false);
     return;
   }
 
-  device_handle_->SetConfiguration(value, std::move(callback));
+  device_state_change_in_progress_ = true;
+  device_handle_->SetConfiguration(
+      value, base::BindOnce(&DeviceImpl::OnSetConfigurationComplete,
+                            weak_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void DeviceImpl::ClaimInterface(uint8_t interface_number,
                                 ClaimInterfaceCallback callback) {
+  if (device_state_change_in_progress_) {
+    mojo::ReportBadMessage("Device state change in progress.");
+    std::move(callback).Run(mojom::UsbClaimInterfaceResult::kFailure);
+    return;
+  }
   if (!device_handle_) {
     std::move(callback).Run(mojom::UsbClaimInterfaceResult::kFailure);
     return;
@@ -390,6 +403,11 @@
 
 void DeviceImpl::ReleaseInterface(uint8_t interface_number,
                                   ReleaseInterfaceCallback callback) {
+  if (device_state_change_in_progress_) {
+    mojo::ReportBadMessage("Device state change in progress.");
+    std::move(callback).Run(false);
+    return;
+  }
   if (!device_handle_) {
     std::move(callback).Run(false);
     return;
@@ -402,6 +420,11 @@
     uint8_t interface_number,
     uint8_t alternate_setting,
     SetInterfaceAlternateSettingCallback callback) {
+  if (device_state_change_in_progress_) {
+    mojo::ReportBadMessage("Device state change in progress.");
+    std::move(callback).Run(false);
+    return;
+  }
   if (!device_handle_) {
     std::move(callback).Run(false);
     return;
@@ -412,12 +435,20 @@
 }
 
 void DeviceImpl::Reset(ResetCallback callback) {
+  if (device_state_change_in_progress_) {
+    mojo::ReportBadMessage("Device state change in progress.");
+    std::move(callback).Run(false);
+    return;
+  }
   if (!device_handle_) {
     std::move(callback).Run(false);
     return;
   }
 
-  device_handle_->ResetDevice(std::move(callback));
+  device_state_change_in_progress_ = true;
+  device_handle_->ResetDevice(base::BindOnce(&DeviceImpl::OnResetComplete,
+                                             weak_factory_.GetWeakPtr(),
+                                             std::move(callback)));
 }
 
 void DeviceImpl::ClearHalt(UsbTransferDirection direction,
@@ -590,6 +621,17 @@
                                   : mojom::UsbClaimInterfaceResult::kFailure);
 }
 
+void DeviceImpl::OnSetConfigurationComplete(SetConfigurationCallback callback,
+                                            bool success) {
+  device_state_change_in_progress_ = false;
+  std::move(callback).Run(success);
+}
+
+void DeviceImpl::OnResetComplete(ResetCallback callback, bool success) {
+  device_state_change_in_progress_ = false;
+  std::move(callback).Run(success);
+}
+
 void DeviceImpl::OnClientConnectionError() {
   // Close the connection with Blink when WebUsbServiceImpl notifies the
   // permission revocation from settings UI.
diff --git a/services/device/usb/mojo/device_impl.h b/services/device/usb/mojo/device_impl.h
index e888a27..54aa1b6 100644
--- a/services/device/usb/mojo/device_impl.h
+++ b/services/device/usb/mojo/device_impl.h
@@ -122,6 +122,9 @@
   void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
 
   void OnInterfaceClaimed(ClaimInterfaceCallback callback, bool success);
+  void OnSetConfigurationComplete(SetConfigurationCallback callback,
+                                  bool success);
+  void OnResetComplete(ResetCallback callback, bool success);
   void OnClientConnectionError();
 
   // Reject and report bad mojo messaage if `length` exceeds limit.
@@ -135,6 +138,7 @@
   // has been closed. |opening_| is set to true while the asynchronous open is
   // in progress.
   bool opening_ = false;
+  bool device_state_change_in_progress_ = false;
   scoped_refptr<UsbDeviceHandle> device_handle_;
 
   const base::flat_set<uint8_t> blocked_interface_classes_;
diff --git a/services/device/usb/mojo/device_impl_unittest.cc b/services/device/usb/mojo/device_impl_unittest.cc
index 0461cd32..1b36def 100644
--- a/services/device/usb/mojo/device_impl_unittest.cc
+++ b/services/device/usb/mojo/device_impl_unittest.cc
@@ -860,6 +860,53 @@
   EXPECT_CALL(mock_handle(), Close());
 }
 
+TEST_F(USBDeviceImplTest, ClaimInterfaceFailsDuringSetConfiguration) {
+  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  {
+    base::test::TestFuture<mojom::UsbOpenDeviceResultPtr> future;
+    device->Open(future.GetCallback());
+    EXPECT_TRUE(future.Get()->is_success());
+  }
+
+  AddMockConfig(ConfigBuilder(1).AddInterface(0, 0, 1, 2, 3).Build());
+
+  UsbDeviceHandle::ResultCallback saved_callback;
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _))
+      .WillOnce([&saved_callback](int value,
+                                  UsbDeviceHandle::ResultCallback& callback) {
+        saved_callback = std::move(callback);
+      });
+
+  // Initiate SetConfiguration but only save the callback without invoking it.
+  base::test::TestFuture<bool> set_config_future;
+  device->SetConfiguration(1, set_config_future.GetCallback());
+
+  // Ensure the request has reached the service.
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(saved_callback);
+
+  // Immediately try to claim interface; should fail synchronously in service.
+  base::test::TestFuture<mojom::UsbClaimInterfaceResult> claim_future;
+  device->ClaimInterface(0, claim_future.GetCallback());
+  EXPECT_EQ(claim_future.Get(), mojom::UsbClaimInterfaceResult::kFailure);
+
+  // Now resolve the pending SetConfiguration.
+  mock_device().ActiveConfigurationChanged(1);
+  std::move(saved_callback).Run(true);
+  EXPECT_TRUE(set_config_future.Get());
+
+  // After SetConfiguration completes, claiming should succeed.
+  EXPECT_CALL(mock_handle(), ClaimInterfaceInternal(0, _));
+  base::test::TestFuture<mojom::UsbClaimInterfaceResult> claim_future2;
+  device->ClaimInterface(0, claim_future2.GetCallback());
+  EXPECT_EQ(claim_future2.Get(), mojom::UsbClaimInterfaceResult::kSuccess);
+
+  EXPECT_CALL(mock_handle(), Close());
+}
+
 TEST_F(USBDeviceImplTest, SetInterfaceAlternateSetting) {
   mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();

diff --git a/services/device/usb/mojo/device_impl_unittest.cc b/services/device/usb/mojo/device_impl_unittest.cc
index 0461cd32..1b36def 100644
--- a/services/device/usb/mojo/device_impl_unittest.cc
+++ b/services/device/usb/mojo/device_impl_unittest.cc
@@ -860,6 +860,53 @@
   EXPECT_CALL(mock_handle(), Close());
 }
 
+TEST_F(USBDeviceImplTest, ClaimInterfaceFailsDuringSetConfiguration) {
+  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  {
+    base::test::TestFuture<mojom::UsbOpenDeviceResultPtr> future;
+    device->Open(future.GetCallback());
+    EXPECT_TRUE(future.Get()->is_success());
+  }
+
+  AddMockConfig(ConfigBuilder(1).AddInterface(0, 0, 1, 2, 3).Build());
+
+  UsbDeviceHandle::ResultCallback saved_callback;
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _))
+      .WillOnce([&saved_callback](int value,
+                                  UsbDeviceHandle::ResultCallback& callback) {
+        saved_callback = std::move(callback);
+      });
+
+  // Initiate SetConfiguration but only save the callback without invoking it.
+  base::test::TestFuture<bool> set_config_future;
+  device->SetConfiguration(1, set_config_future.GetCallback());
+
+  // Ensure the request has reached the service.
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(saved_callback);
+
+  // Immediately try to claim interface; should fail synchronously in service.
+  base::test::TestFuture<mojom::UsbClaimInterfaceResult> claim_future;
+  device->ClaimInterface(0, claim_future.GetCallback());
+  EXPECT_EQ(claim_future.Get(), mojom::UsbClaimInterfaceResult::kFailure);
+
+  // Now resolve the pending SetConfiguration.
+  mock_device().ActiveConfigurationChanged(1);
+  std::move(saved_callback).Run(true);
+  EXPECT_TRUE(set_config_future.Get());
+
+  // After SetConfiguration completes, claiming should succeed.
+  EXPECT_CALL(mock_handle(), ClaimInterfaceInternal(0, _));
+  base::test::TestFuture<mojom::UsbClaimInterfaceResult> claim_future2;
+  device->ClaimInterface(0, claim_future2.GetCallback());
+  EXPECT_EQ(claim_future2.Get(), mojom::UsbClaimInterfaceResult::kSuccess);
+
+  EXPECT_CALL(mock_handle(), Close());
+}
+
 TEST_F(USBDeviceImplTest, SetInterfaceAlternateSetting) {
   mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();