Fix exFAT-on-GPT USB drives (and a UMS device-list use-after-free)

.gitignore

@@ -57,6 +57,7 @@ build*/
 
 misc/*/*
 !misc/*/PKGBUILD
+!misc/*/*.patch
 !misc/crossfile.txt
 
 assets/icon.svg

Dockerfile

@@ -26,11 +26,13 @@ ENV DEVKITPRO=/opt/devkitpro
 ENV PORTLIBS_PREFIX=${DEVKITPRO}/portlibs/switch
 
 # Build libusbhsfs (GPL)
+COPY misc/libusbhsfs/relaxed-gpt-vbr-probe.patch /tmp/libusbhsfs-relaxed-gpt-vbr-probe.patch
 RUN git clone --depth 1 https://github.com/DarkMatterCore/libusbhsfs.git /tmp/libusbhsfs \
     && cd /tmp/libusbhsfs \
+    && patch -Np0 -i /tmp/libusbhsfs-relaxed-gpt-vbr-probe.patch \
     && source ${DEVKITPRO}/switchvars.sh \
     && make BUILD_TYPE=gpl install \
-    && rm -rf /tmp/libusbhsfs
+    && rm -rf /tmp/libusbhsfs /tmp/libusbhsfs-relaxed-gpt-vbr-probe.patch
 
 # Build libsmb2
 RUN git clone --depth 1 https://github.com/sahlberg/libsmb2.git /tmp/libsmb2 \

Makefile

@@ -51,7 +51,12 @@ CFLAGS                  :=  -std=gnu11
 CXXFLAGS                :=  -std=gnu++23
 ASFLAGS                 :=
 LDFLAGS                 :=  -g -Wl,--gc-sections -Wl,-pie -specs=$(DEVKITPRO)/libnx/switch.specs
-LINKS                   :=  -lusbhsfs -lntfs-3g -llwext4 -ldeko3d -lnx
+LIBUSBHSFS_DEBUG        ?=  0
+LIBUSBHSFS_LINK         :=  -lusbhsfs
+ifeq ($(LIBUSBHSFS_DEBUG),1)
+LIBUSBHSFS_LINK         :=  -lusbhsfsd
+endif
+LINKS                   :=  $(LIBUSBHSFS_LINK) -lntfs-3g -llwext4 -ldeko3d -lnx
 PREFIX                  :=  aarch64-none-elf-
 
 # -----------------------------------------------

build-docker.sh

@@ -17,5 +17,5 @@ docker run --rm --name devkitpro-switchwave \
         make build-uam
         make configure-mpv
         make build-mpv
-        make dist -j\$(nproc)
+        make dist -j\$(nproc) LIBUSBHSFS_DEBUG=${LIBUSBHSFS_DEBUG:-0}
     "

misc/libusbhsfs/relaxed-gpt-vbr-probe.patch

@@ -0,0 +1,51 @@
+--- source/usbhsfs_mount.c
++++ source/usbhsfs_mount.c
+@@ -212,6 +212,7 @@ static Mutex g_devoptabDefaultDeviceMutex = 0;
+
+ static const u8 g_microsoftBasicDataPartitionGuid[0x10] = { 0xA2, 0xA0, 0xD0, 0xEB, 0xE5, 0xB9, 0x33, 0x44, 0x87, 0xC0, 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7 };   /* EBD0A0A2-B9E5-4433-87C0-68B6B72699C7. */
+ static const u8 g_linuxFilesystemDataGuid[0x10] = { 0xAF, 0x3D, 0xC6, 0x0F, 0x83, 0x84, 0x72, 0x47, 0x8E, 0x79, 0x3D, 0x69, 0xD8, 0x47, 0x7D, 0xE4 };           /* 0FC63DAF-8483-4772-8E79-3D69D8477DE4. */
++static const u8 g_emptyPartitionGuid[0x10] = {0};
+
+ static u32 g_fileSystemMountFlags = UsbHsFsMountFlags_Default;
+
+@@ -714,6 +715,16 @@ static void usbHsFsMountParseGuidPartitionTableEntry(UsbHsFsDriveLogicalUnitCont
+     u64 entry_size = ((gpt_entry->lba_end + 1) - gpt_entry->lba_start);
+     UsbHsFsDriveLogicalUnitFileSystemType fs_type = UsbHsFsDriveLogicalUnitFileSystemType_Invalid;
+
++    /* Discard entries whose LBA span falls outside this logical unit before any branch below issues a read. */
++    /* This filters out uninitialized/garbage partition array entries (e.g. a type GUID 0xF4-filled with an */
++    /* lba_start of 0xF4F4F4F4F4F4F4F4), whose reads would target absurd LBAs that can stall flaky USB drives, */
++    /* force a BOT reset and knock them off the bus. */
++    if (entry_lba >= lun_ctx->block_count || gpt_entry->lba_end >= lun_ctx->block_count || gpt_entry->lba_end < entry_lba)
++    {
++        USBHSFS_LOG_MSG("Discarding GPT partition entry with out-of-range LBA span (0x%lX - 0x%lX) (interface %d, LUN %u).", entry_lba, gpt_entry->lba_end, lun_ctx->usb_if_id, lun_ctx->lun);
++        return;
++    }
++
+     if (!memcmp(gpt_entry->type_guid, g_microsoftBasicDataPartitionGuid, sizeof(g_microsoftBasicDataPartitionGuid)))
+     {
+         /* We're dealing with a Microsoft Basic Data Partition entry. */
+@@ -738,6 +749,23 @@ static void usbHsFsMountParseGuidPartitionTableEntry(UsbHsFsDriveLogicalUnitCont
+ #ifdef GPL_BUILD
+         /* Check if this LBA points to a valid EXT superblock. Register the EXT volume if so. */
+         fs_type = usbHsFsMountInspectExtSuperBlock(lun_ctx, block, entry_lba);
++#endif
++    } else
++    if (memcmp(gpt_entry->type_guid, g_emptyPartitionGuid, sizeof(g_emptyPartitionGuid)))
++    {
++        /* This entry carries a non-empty type GUID that none of the branches above recognised; entries with an */
++        /* all-zero (empty) type GUID are unused and skipped here. Some tools place standard exFAT/FAT/NTFS (or */
++        /* even EXT) volumes behind such GUIDs -- e.g. the Windows Recovery Environment partition created by */
++        /* Windows Setup uses type GUID DE94BBA4-06D1-4D40-A16A-BFD50179D6AC yet holds an NTFS volume. Probe the */
++        /* VBR (and, on GPL builds, the EXT superblock) before skipping the entry. */
++        USBHSFS_LOG_MSG("Found unrecognized GPT partition type at LBA 0x%lX. Probing VBR (interface %d, LUN %u).", entry_lba, lun_ctx->usb_if_id, lun_ctx->lun);
++        fs_type = usbHsFsMountInspectVolumeBootRecord(lun_ctx, block, entry_lba);
++#ifdef GPL_BUILD
++        if (fs_type == UsbHsFsDriveLogicalUnitFileSystemType_Invalid)
++        {
++            /* The entry may instead point to an EXT volume sitting behind a non-standard GUID. */
++            fs_type = usbHsFsMountInspectExtSuperBlock(lun_ctx, block, entry_lba);
++        }
+ #endif
+     }
+

src/fs/fs_ums.hpp

@@ -1,6 +1,9 @@
 #pragma once
 
 #include <cstdint>
+#include <mutex>
+#include <string>
+#include <string_view>
 #include <vector>
 
 #include <usbhsfs.h>
@@ -13,16 +16,21 @@ class UmsController {
             UsbHsFsDeviceFileSystemType type;
             std::int32_t intf_id;
             std::string name, mount_name;
+
+            bool operator==(const Device &) const = default;
         };
 
         using DevicesChangedCallback = void(*)(const std::vector<Device> &, void *);
 
     public:
         Result initialize() {
+            usbHsFsSetFileSystemMountFlags(UsbHsFsMountFlags_ReadOnly);
+
             if (auto rc = usbHsFsInitialize(0); R_FAILED(rc))
                 return rc;
 
             usbHsFsSetPopulateCallback(UmsController::usbhsfs_populate_cb, this);
+            this->refresh_devices(false);
 
             return 0;
         }
@@ -31,41 +39,76 @@ class UmsController {
             usbHsFsSetPopulateCallback(nullptr, nullptr);
             this->set_devices_changed_callback(nullptr, nullptr);
 
-            for (auto &dev: this->devices)
+            auto devices = this->get_devices();
+            for (auto &dev: devices)
                 this->unmount_device(dev);
             usbHsFsExit();
         }
 
         void set_devices_changed_callback(DevicesChangedCallback cb, void *user = nullptr) {
-            this->devices_changed_cb = cb, this->devices_changed_user = user;
-        }
+            std::vector<Device> devices;
+            DevicesChangedCallback cb_to_call = nullptr;
+            void *user_to_call = nullptr;
+
+            {
+                auto lk = std::scoped_lock(this->devices_mtx);
+                this->devices_changed_cb = cb, this->devices_changed_user = user;
+
+                if (this->devices_changed_cb) {
+                    devices = this->devices;
+                    cb_to_call = this->devices_changed_cb;
+                    user_to_call = this->devices_changed_user;
+                }
+            }
 
-        std::uint32_t get_num_filesystems() const {
-            return usbHsFsGetMountedDeviceCount();
+            if (cb_to_call)
+                cb_to_call(devices, user_to_call);
         }
 
-        const std::vector<Device> &get_devices() const {
+        std::vector<Device> get_devices() const {
+            auto lk = std::scoped_lock(this->devices_mtx);
             return this->devices;
         }
 
+        void refresh_devices(bool notify_changed = true) {
+            auto device_count = usbHsFsGetMountedDeviceCount();
+
+            std::vector<UsbHsFsDevice> devices(device_count);
+            if (device_count) {
+                device_count = usbHsFsListMountedDevices(devices.data(), devices.size());
+                if (device_count > devices.size())
+                    device_count = devices.size();
+            }
+
+            this->populate_devices(devices.data(), device_count, notify_changed);
+        }
+
         bool unmount_device(const Device &dev) {
             UsbHsFsDevice d = {
                 .usb_if_id = dev.intf_id,
             };
 
-            std::erase_if(this->devices, [&dev](const auto &d) {
-                return d.mount_name == dev.mount_name;
-            });
+            {
+                auto lk = std::scoped_lock(this->devices_mtx);
+                std::erase_if(this->devices, [&dev](const auto &d) {
+                    return d.mount_name == dev.mount_name;
+                });
+            }
 
             return usbHsFsUnmountDevice(&d, true);
         }
 
     private:
         static void usbhsfs_populate_cb(const UsbHsFsDevice *devices, u32 device_count, void *user_data) {
             auto *self = static_cast<UmsController *>(user_data);
+            self->populate_devices(devices, device_count, true);
+        }
 
-            self->devices.clear();
-            self->devices.reserve(device_count);
+        void populate_devices(const UsbHsFsDevice *devices, u32 device_count, bool notify_changed) {
+            std::vector<Device> new_devices;
+            new_devices.reserve(device_count);
+            DevicesChangedCallback cb_to_call = nullptr;
+            void *user_to_call = nullptr;
 
             for (u32 i = 0; i < device_count; ++i) {
                 auto &d = devices[i];
@@ -80,15 +123,31 @@ class UmsController {
                 else
                     name = "Unnamed device";
 
-                self->devices.emplace_back(UsbHsFsDeviceFileSystemType(d.fs_type), d.usb_if_id, std::move(name), d.name);
+                new_devices.emplace_back(UsbHsFsDeviceFileSystemType(d.fs_type), d.usb_if_id, std::move(name), d.name);
+            }
+
+            {
+                auto lk = std::scoped_lock(this->devices_mtx);
+
+                if (new_devices == this->devices)
+                    return;
+
+                this->devices = std::move(new_devices);
+                new_devices = this->devices;
+
+                if (notify_changed) {
+                    cb_to_call = this->devices_changed_cb;
+                    user_to_call = this->devices_changed_user;
+                }
             }
 
-            if (self->devices_changed_cb)
-                self->devices_changed_cb(self->devices, self->devices_changed_user);
+            if (cb_to_call)
+                cb_to_call(new_devices, user_to_call);
         }
 
     private:
         UEvent *status_event = nullptr;
+        mutable std::mutex devices_mtx;
         std::vector<Device> devices;
 
         DevicesChangedCallback devices_changed_cb = nullptr;

src/ui/ui_main_menu.cpp

@@ -980,25 +980,26 @@ void SettingsEditor::render() {
         ImGui::TableSetupColumn("Status", ImGuiTableColumnFlags_WidthFixed, this->screen_rel_width(0.1));
         ImGui::TableHeadersRow();
 
-        auto &devs = this->context.ums.get_devices();
+        auto devs = this->context.ums.get_devices();
         for (std::size_t i = 0; i < devs.size(); ++i) {
             auto &dev = devs[i];
 
             if (i > 0)
                 ImGui::TableNextRow();
 
             ImGui::TableNextColumn();
-            ImGui::Text(dev.name.c_str());
+            ImGui::Text("%s", dev.name.c_str());
 
             ImGui::TableNextColumn();
-            ImGui::Text(LIBUSBHSFS_FS_TYPE_STR(dev.type));
+            ImGui::Text("%s", LIBUSBHSFS_FS_TYPE_STR(dev.type));
 
             ImGui::TableNextColumn();
             if (ImGui::Button(make_id(i, "Unmount"))) {
                 std::erase_if(this->context.filesystems, [&dev](const auto &fs) {
                     return dev.mount_name == fs->mount_name;
                 });
-                this->context.cur_fs = this->context.filesystems.front();
+                if (!this->context.filesystems.empty())
+                    this->context.cur_fs = this->context.filesystems.front();
 
                 context.ums.unmount_device(dev);
             }