[WiiU / vWii] WiiFlow may mistakenly mount WiiU's USB storage drive

The loop in usbstorage_libogc#__usbstorage_ogc_IsInserted (referenced in the first link below) breaks as soon as it finds any USB storage device. If the Wii U USB drive happens to appear ahead of the Wii USB drive in the usb_device_entry buffer, WiiFlow will attempt to use it as the USB partition. This explains why some people cannot access their Wii USB drive without first unplugging their WiiU USB drive while others have no problems leaving both drives connected.

https://github.com/Fledge68/WiiFlow_Lite/blob/2fd07be7a8538679e24f4ed1fc7f35960bf639ad/source/devicemounter/usbstorage_libogc.c#L877-L909

To verify this bug, the code above can be modified to ignore the device whose vid and pid match the Wii U's USB drive. In my case, I added the following condition:

  if (vid == 0x04E8 && pid == 0x61F5) // Wii U SSD
            continue;

This causes the loop to skip my Wii U USB drive and move on to my Wii USB drive. Obviously, the code snippet above is specific to my setup and is not a generic fix for the issue since it arbitrarily assumes that a particular model of USB drive is always used as the Wii U's USB storage when it could just as easily be used as the Wii's USB drive.

Workaround: By setting "Force Load cIOS" WiiFlow does not use libogc (see code referenced below). This allowed me to access my Wii USB drive immediately.

https://github.com/Fledge68/WiiFlow_Lite/blob/6f6b3ec2d847ee163fdbaf43698be5faa60ec6ea/source/devicemounter/DeviceHandler.cpp#L55-L64

Potential fixes: Add some sanity checking to __usbstorage_ogc_IsInserted to ignore USB drives with the Wii U partition type.

Questions: Why is usbstorage_libogc.c in the WiiFlow repo? It's a near duplicate of usbstorage.c in the devkitPro repo with some function and variable names slightly altered. It's possible some of the functions/variables in the devkitPro file may collide with existing functions/variables in WiiFlow. If that's the case, the WiiFlow-specific names should probably have been changed to avoid collision with devkitPro rather than modifying a copy of the devkitPro file. This would allow WiiFlow to reference devkitPro and automatically pick up any updates / bug fixes in the file without needing to re-import and re-modify it.

all very good info. i appreciate that you found the problem. this looks to be a little tricky to find a solution. i'll keep looking when i can.

by the way wiiflow does not use usbstorage.c in devkitpro. this prevents any conflicts there. not sure why the devs of long ago coded it this way but it works except for your issue. i suppose we could force wii u users to use a cios and not allow the use of IOS58.

Just following up on this, I coded up a slightly ugly implementation of the potential fix mentioned in the issue description. The fix adds a sanity check to the loop in __usbstorage_ogc_IsInserted which reads the MBR sector of the drive and skips over drives whose partition type is "Unknown" (according to the logic in the PartFromType function defined in PartitionHandle.cpp).

Ideally, this could have been implemented fairly cleanly by reusing the PARTITION_RECORD and MASTER_BOOT_RECORD structs defined in PartitionHandle.h. However, since PartitionHandle.h is a cpp source file, I was unable to reference these structs from within usbstorage_libogc.c. Instead, I accessed the raw byte containing the partition type directly by counting the number of bytes in the PARTITION_RECORD struct (16 bytes) and calculating the offset of the "partition type byte's" location within the MBR sector. The code looks like this:

u8 mbr = (u8)lwp_heap_allocate(&heap, 512); USBStorage_OGC_Read(&usbfd, j, 0, 1, mbr); bool readablePartition = false; for (u8 i = 0; i < 4; i++) { // The "partition type" byte for each partition on the drive // is located at 16 byte intervals beginning at offset 450 u8 rawPartitionType = mbr[450 + i 16]; const char partitionType = PartFromType(rawPartitionType); if (strcmp(partitionType, "Unknown") != 0) { readablePartition = true; } } lwp_heap_free(&__heap, mbr);

        if (!readablePartition) {
            gprintf("USB storage device with vid %lu pid %lu has no readable partitions. Skipping...\n", vid, pid);
            __usbstorage_ogc_reset(&__usbfd);
            continue;
        }

To tidy this up, the missing structs could be added to usbstorage_libogc.c (or usbstorage_libogc.h) and macros could be defined for the hardcoded numbers (e.g. BYTES_PER_SECTOR). Also, I needed to copy the PartFromType function defined in PartitionHandle.cpp into usbstorage_libogc.c since it is defined in a cpp source file as well.

I have a custom build of WiiFlow running this code on my Wii U and the fix can be viewed in its entirety in this commit of my fork of the WiiFlow repo. I can create a pull request with my changes so they can be integrated here if requested. However, there are a few potential reasons not to integrate the code (at least in its current form) that are worth considering.

First, as mentioned in the original issue comment, if you compare the usbstorage_libogc.c source file contained in the WiiFlow repo to the libogc\usbstorage.c file contained in the devkitPro repo you'll see that, aside from a handful of renamed variables and functions, they're nearly identical. For example, __usbstorage_clearerrors in devkitPro has been renamed to __usbstorage_ogc_clearerrors in WiiFlow but its contents are unchanged. One possible explanation for why libogc\usbstorage.c was copied into the WiiFlow repository and altered like this instead of being referenced and linked at compile time would be if some of the function and/or variable names in devkitPro conflict with the functions and variables in WiiFlow's "other" usbstorage.c source file. At a glance, I couldn't find any glaring conflicts (aside from the obvious conflict in the source file name itself -- usbstorage.c is used in both repos). If I were the maintainer of this project, I'd be hesitant to introduce code to usbstorage_libogc.c that causes it to diverge further from the implementation in devkitPro because it would make pulling in future fixes from the devkitPro repo more likely to cause merge conflicts. The ideal solution to this problem is most likely to remove WiiFlow's modified copy of usbstorage_libogc.c from the repo entirely and instead reference devkitPro's implementation during the build. If that approach were pursued, my solution would need to be integrated into devkitPro/libogc/usbstorage.c rather than here in the WiiFlow repo.

Second, the workaround of setting "force load cIOS" to "on" might be acceptable to most users. I'm not sure what (if any) benefits are associated with using libogc over cIOS on Wii U. If force loading a cIOS adds another layer to the IO path, it's possible using libogc would improve performance. However, without performance data to confirm this or at minimum some additional Wii U users' anecdotal experience that using a cIOS is noticeably slower, the alternate solution of automatically setting "force load cIOS" to "on" when running WiiFlow on a vWii may be simpler and cleaner.

One other curious note, the latest version of usbloadergx is able to read from the Wii USB drive while the Wii U drive is attached by default, without any modifications to its settings. That may mean that the usbloadergx code already contains logic to skip over Wii U storage drives which could be ported to WiiFlow and reused (of course, it's also possible that usbloadergx just gets lucky and iterates through the storage drives in the opposite order). I took a brief look at the usbloadergx source while working on this fix and nothing jumped out at me but it's possible someone with more experience working on usbloadergx (maybe blackb0x) could explain how it correctly handles Wii U storage drives.

As for the future of this issue, it would be awesome if a fix that allows Wii U users to access their Wii USB storage drive without unplugging their Wii U storage drive or setting "force load cIOS" to "on" were integrated into either WiiFlow or devkitPro but it's totally understandable if that's not feasible, in which case I'm content using my custom WiiFlow build indefinitely (and could upload the boot.dol file to my fork if others want to use it). Regardless, this issue should probably be left open to document the bug and the workarounds in case other Wii U users are struggling to understand why data on their Wii USB drive isn't showing up in WiiFlow.

@kcpants are you going to provide a pull request?

all very good information/analysis. i looked over usbloadergx code and could not find any obvious reason why it works and wiiflow doesn't. looking at wiiflow code I did find where Fix94 added libogc USB and SD code to wiiflow. It was when IOS58 USB support was found and Fix94 added it to wiiflow. As to why he added parts of libogc i'm not sure. i believe he omitted some of its code maybe in an attempt to keep wiiflow's boot.dol small in size.

anyway, i looked at your commit and don't see any problem with it. if you'd like you can send a PR or i can just add it and give you credit.

Sure, I've created a PR here. I also opened an issue in devkitPro to make its maintainers aware of the issue in case they'd like to fix it there as well.

PR accepted. issue closed but will keep open for a while for any further comments.

@kcpants i saw your issue on libogc. its a shame DacoTaco stopped talking after march 21. have you heard anymore since then? maybe in private? I have a feeling he dropped it because it it required too much rewriting of the code, wintermute didn't want to pursue a solution, or he was afraid it would break something else. i'm fine with your patch but do realize it won't work for GPT drives. the best solution is to just force wfl to use a cIOS instead of IOS58.

@Fledge68 I did not discuss the issue with anyone beyond what is in the public comments. Handling GPT is certainly a bit intimidating and it probably would be necessary to do in some capacity if changes were made to libogc since other apps that currently rely on libogc may support GPT storage drives.

As for my patch in WiiFlow, I spent the day thinking about it, looking through the rest of the code, doing a bit of research into GPT, and running a few tests and came to the conclusion that my code needs a small tweak but otherwise is still probably worth keeping even if you do decide to change the default to "Force Load cIOS".

TL;DR I updated my WiiFlow patch to support GPT and learned a bit about GPT, the proprietary WiiU HDD format, and how WiiFlow interacts with GPT drives along the way.

First, the reason why I think including a fix for this issue makes sense regardless of what the default setting is for "Force Load cIOS" is that it's very hard to determine whether this problem will occur on any given setup making it unpredictable and potentially very frustrating for users. Since the bug only appears if the WiiU USB drive happens to be ordered ahead of the Wii USB drive when storage drives are scanned, it's possible that someone with a working setup (who has "Force Load cIOS" off and got lucky to avoid the bug) could replace a USB drive with a new one which inadvertently alters the drive order causing the WiiU drive to be read first. A user in this situation might logically think that there's something wrong with the new USB drive itself (since their old one was working). It's not obvious at all that switching to "Force Load cIOS" would solve their problem and they wont see any error messages to indicate that they are reading from the wrong drive (just that they cannot read from the usb1:/games/ directory).

Now, the obvious problem with my patch is that it ignores GPT drives and although there are reports that suggest GPT drives can cause problems for homebrew apps there is also some (mis)information out there that a GPT drive is actually required for reading Gamecube isos which means some people are probably using GPT drives and that breaking GPT support in WiiFlow would likely cause problems (the claim that Nintendont requires GPT to load gamecube isos is refuted in this Gamebrew Wiki).

Since the WiiU USB storage drive is laid out in a proprietary format, it occurred to me that if it doesn't present as either MBR or GPT it might be possible to write code that simply ignores drives that aren't GPT or MBR. A solution like this would not need to deal with the complexity of the partition table at all. I connected my WiiU storage drive to a Windows PC and examined its raw bytes in a hex editor and it did not have an MBR signature or a GPT signature which was promising.

vvvEDITvvv: The result described below is not reproducible and is likely specious

Unfortunately, when I coded up the fix and added it to WiiFlow I found that WiiFlow believed the drive did, in fact, have a GPT signature. This implies that the WiiU proprietary format is encrypted GPT (decrypted by the WiiU on startup so apps running within the WiiU can access the data).

^^^EDIT^^^: The result described above is not reproducible and is likely specious

After hitting this roadblock, I decided to look into how WiiFlow supports GPT and while reading through the source I stumbled on the following code in PartitionHandle::findPartitions (that was added for UStealth support) which assumes that the attached drive is MBR and returns -1 if it is not: https://github.com/Fledge68/WiiFlow_Lite/blob/cc9aa985edaa9ad7cf2f25e18dca92b19fc289b3/source/devicemounter/PartitionHandle.cpp#L265-L269 This logic causes WiiFlow to exit the findPartitions function before ever calling PartitionHandle::AddPartition which seems to imply that the PartitionList vector will always be empty for GPT drives and any calls to GetPartitionCount would return 0. https://github.com/Fledge68/WiiFlow_Lite/blob/12d21fa0a7362fefd84313815e6af7ec32415b26/source/devicemounter/PartitionHandle.h#L152 If GetUSBPartitionCount returns 0 then the result boolean in the following code will always be false and MountUSB is never called. After all these failures, DeviceHandler would make one last-ditch effort to mount the drive via a call to usb.Mount(0, DeviceName[USB1], true); /* Force FAT */ which passes the drive to libfat and hopes it can find a FAT partition. https://github.com/Fledge68/WiiFlow_Lite/blob/9350f51c3b7fd5fabcd6206014f3ef115c5eb2da/source/devicemounter/DeviceHandler.cpp#L157-L168

So on the surface, it looks like GPT support in WiiFlow all relies on that final call to usb.Mount(0, DeviceName[USB1], true); /* Force FAT */. However, looking a little further in PartitionHandle::findPartitions we see some code that deals with GPT drives -- immediately after the MBR check! https://github.com/Fledge68/WiiFlow_Lite/blob/12d21fa0a7362fefd84313815e6af7ec32415b26/source/devicemounter/PartitionHandle.cpp#L264-L280 With my limited knowledge of the MBR and GPT layouts, to me this looked like dead code that could never be executed. However, it turns out that the GPT specification reserves the space used by MBR which it fills with a "protective MBR". From Wikipedia:

Protective MBR (LBA 0) For limited backward compatibility, the space of the legacy Master Boot Record (MBR) is still reserved in the GPT specification, but it is now used in a way that prevents MBR-based disk utilities from misrecognizing and possibly overwriting GPT disks. This is referred to as a protective MBR.

A single partition of type EEh, encompassing the entire GPT drive (where "entire" actually means as much of the drive as can be represented in an MBR), is indicated and identifies it as GPT.

This means that GPT drives should all have an MBR signature. However, the WiiU formatted USB drive does not have an MBR signature so this seemed like it could be a way to determine which drive to skip.

To test all this, I created a GPT USB drive and put some isos on it. I found that a GPT USB drive with a protective MBR can be accessed by WiiFlow but a GPT USB drive without a protective MBR cannot be accessed by WiiFlow (I added some logging and confirmed that in this case PartitionList is empty and the libfat mount fails as expected). For completeness, I also found that if the SD card is GPT WiiFlow and usbLoaderGX won't even run and the homebrew channel is unable to find any apps.

The takeaway of all this is:

GPT specification includes space for a "protective MBR" to prevent older systems from thinking GPT drives are empty
WiiU drives do not have a protective MBR
WiiFlow fails to mount GPT drives without a protective MBR

Meaning to fix this issue it is only necessary to check for the MBR signature (as well as the modified signature used by UStealth) and ignore drives that don't have one. This is guaranteed not to cause regressions because testing confirms that without my patch WiiFlow can only access GPT drives with a protective MBR so ignoring drives without an MBR signature will never exclude anyone's working storage drive.

I will create a pull request that backs out most of my original patch and includes the new change in case you'd like to integrate it and will let the devkitPro folks know that they may be able to solve the issue in libogc without writing code to read partition tables. If we're lucky, maybe dacoTaco will be inspired to pick it back up!

once again good analysis. I knew about the protective MBR on GPT drives. i just didn't know about a wii u hdd structure. I will look at your PR but right at the moment i'm working on possibly moving ext_booter.bin code back into wiiflow so that with the newest devkitppc and libogc wfl can boot wii and channel games. and hopefully the time commit in libogc will fix wii fit plus time and date issue.

edit: just looked at your commit. very nice and simple. just adds a MBR signature check.

i just didn't know about a wii u hdd structure

There's not a lot of specific information about the WiiU drive layout online. In the end, reading and printing the contents of the first few LBAs of the WiiU's storage drive turned out to be the key to cracking this fix.

As always, thanks for the quick feedback and for being an excellent steward for this open source project!

Unrelated to this issue, have you thought about adding the build artifacts that are created by make to the .gitignore? Since they aren't always rebuilt and added with each commit, they wont always be in sync with the source and because they are relatively large compared to the source files, over time they will cause the repo to grow much faster than it otherwise would (source files are a few KB while the dols are a few MB so the repo will grow 1000 times faster if the build artifacts are included). Of course, adding the binaries to the .gitignore means you could only distribute updated versions of WiiFlow using github releases and anyone who wants updates between releases would need to pull the source and make the dols themselves.

Fledge68 / WiiFlow_Lite

[WiiU / vWii] WiiFlow may mistakenly mount WiiU's USB storage drive #338