Skip to content

Concurrent data structure access without mutual exclusion #688

@Allen-Webb

Description

@Allen-Webb

See: https://chromium-review.git.corp.google.com/c/chromiumos/overlays/chromiumos-overlay/+/7663965/1/sys-apps/usbguard/files/fix-race-conditions.patch#12](https://chromium-review.git.corp.google.com/c/chromiumos/overlays/chromiumos-overlay/+/7663965)

The _sysfs_path_to_id_map and _backlog structures were being accessed
concurrently by the main thread and the uevent thread without proper
synchronization. This could lead to memory corruption or erroneous
device rejections during the initial system scanning phase.

Fix this by:
1. Protecting _sysfs_path_to_id_map in DeviceManagerBase using the
   existing refDeviceMapMutex().
2. Protecting the event _backlog in UEventDeviceManager using a
   dedicated static mutex to ensure thread-safe buffering and handoff.
diff --git a/src/Library/DeviceManagerBase.cpp b/src/Library/DeviceManagerBase.cpp
index b2bcc94..e4a564d 100644
--- a/src/Library/DeviceManagerBase.cpp
+++ b/src/Library/DeviceManagerBase.cpp
@@ -95,6 +95,7 @@ namespace usbguard
 
   uint32_t DeviceManagerBase::getIDFromSysfsPath(const std::string& sysfs_path) const
   {
+    std::lock_guard<std::mutex> lock(const_cast<DeviceManagerBase*>(this)->refDeviceMapMutex());
     uint32_t id = 0;
 
     if (knownSysfsPath(sysfs_path, &id)) {
@@ -356,6 +357,7 @@ namespace usbguard
 
   bool DeviceManagerBase::isPresentSysfsPath(const std::string& sysfs_path) const
   {
+    std::lock_guard<std::mutex> lock(const_cast<DeviceManagerBase*>(this)->refDeviceMapMutex());
     uint32_t id = 0;
 
     if (knownSysfsPath(sysfs_path, &id)) {
@@ -367,6 +369,7 @@ namespace usbguard
 
   bool DeviceManagerBase::knownSysfsPath(const std::string& sysfs_path, uint32_t* id_ptr) const
   {
+    std::lock_guard<std::mutex> lock(const_cast<DeviceManagerBase*>(this)->refDeviceMapMutex());
     USBGUARD_LOG(Trace) << "Known? sysfs_path=" << sysfs_path << " size=" << sysfs_path.size() << " id_ptr=" << (void*)id_ptr;
     auto it = _sysfs_path_to_id_map.find(sysfs_path);
     uint32_t known_id = 0;
@@ -388,12 +391,14 @@ namespace usbguard
 
   void DeviceManagerBase::learnSysfsPath(const std::string& sysfs_path, uint32_t id)
   {
+    std::lock_guard<std::mutex> lock(refDeviceMapMutex());
     USBGUARD_LOG(Trace) << "Learn sysfs_path=" << sysfs_path << " size=" << sysfs_path.size() << " id=" << id;
     _sysfs_path_to_id_map[sysfs_path] = id;
   }
 
   void DeviceManagerBase::forgetSysfsPath(const std::string& sysfs_path)
   {
+    std::lock_guard<std::mutex> lock(refDeviceMapMutex());
     USBGUARD_LOG(Trace) << "Forget sysfs_path=" << sysfs_path;
     _sysfs_path_to_id_map.erase(sysfs_path);
   }
diff --git a/src/Library/DeviceManagerPrivate.cpp b/src/Library/DeviceManagerPrivate.cpp
index 73f84fd..6daa21b 100644
--- a/src/Library/DeviceManagerPrivate.cpp
+++ b/src/Library/DeviceManagerPrivate.cpp
@@ -122,6 +122,11 @@ namespace usbguard
     }
   }
 
+  std::mutex& DeviceManagerPrivate::refDeviceMapMutex()
+  {
+    return _device_map_mutex;
+  }
+
   void DeviceManagerPrivate::DeviceEvent(DeviceManager::EventType event, std::shared_ptr<Device> device)
   {
     USBGUARD_LOG(Trace) << "event=" << DeviceManager::eventTypeToString(event)
diff --git a/src/Library/UEventDeviceManager.cpp b/src/Library/UEventDeviceManager.cpp
index 8d1fb79..c634724 100644
--- a/src/Library/UEventDeviceManager.cpp
+++ b/src/Library/UEventDeviceManager.cpp
@@ -45,8 +45,11 @@
 #include <limits.h>
 #include <stdlib.h>
 
+#include <pthread.h>
+
 namespace usbguard
 {
+  static pthread_mutex_t G_backlog_mutex = PTHREAD_MUTEX_INITIALIZER;
 
   UEventDeviceManager::UEventDeviceManager(DeviceManagerHooks& hooks)
     : DeviceManagerBase(hooks),
@@ -332,7 +335,9 @@ namespace usbguard
     const std::string sysfs_devpath = uevent.getAttribute("DEVPATH");
 
     if (_enumeration) {
+      pthread_mutex_lock(&G_backlog_mutex);
       _backlog.emplace_back(std::move(uevent));
+      pthread_mutex_unlock(&G_backlog_mutex);
     }
     else {
       ueventProcessAction(action, sysfs_devpath);
@@ -453,10 +458,16 @@ namespace usbguard
 
   void UEventDeviceManager::processBacklog()
   {
-    USBGUARD_LOG(Debug) << "Processing backlog: _backlog.size() = " << _backlog.size();
+    std::vector<UEvent> backlog_copy;
+    {
+      pthread_mutex_lock(&G_backlog_mutex);
+      backlog_copy = std::move(_backlog);
+      pthread_mutex_unlock(&G_backlog_mutex);
+    }
+    USBGUARD_LOG(Debug) << "Processing backlog: backlog_copy.size() = " << backlog_copy.size();
 
     try {
-      for (auto& it : _backlog) {
+      for (auto& it : backlog_copy) {
         ueventProcessUEvent(std::move(it));
       }
     }
diff --git a/src/Library/public/usbguard/DeviceManager.cpp b/src/Library/public/usbguard/DeviceManager.cpp
index c7c3784..c8f32f1 100644
--- a/src/Library/public/usbguard/DeviceManager.cpp
+++ b/src/Library/public/usbguard/DeviceManager.cpp
@@ -206,6 +206,11 @@ namespace usbguard
     return d_pointer->getDevice(id);
   }
 
+  std::mutex& DeviceManager::refDeviceMapMutex()
+  {
+    return d_pointer->refDeviceMapMutex();
+  }
+
   void DeviceManager::DeviceEvent(DeviceManager::EventType event, std::shared_ptr<Device> device)
   {
     d_pointer->DeviceEvent(event, device);

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions