[WiiU / vWii] libogc may mistakenly mount WiiU's USB storage drive when multiple USB drives are connected

[kcpants]

The loop in usbstorage.c#__usbstorage_IsInserted (referenced in the first link below) breaks as soon as it finds any USB storage device. If a drive with an unreadable proprietary format (e.g. a Wii U formatted drive) happens to appear ahead of a readable drive (e.g. a FAT32 Wii USB drive) in the usb_device_entry buffer, usbstorage.c will mount it and exit the function. This may cause issues in higher level apps which rely on this function to mount a USB storage drive when multiple USB storage drives are connected and some subset of them are in an unreadable proprietary format.

libogc/libogc/usbstorage.c

Lines 879 to 917 in 46b47a0

	for (i = 0; i < device_count; i++) {
	vid = buffer[i].vid;
	pid = buffer[i].pid;
	if (vid == 0 || pid == 0)
	continue;
	if (vid == 0x0b95 && pid == 0x7720) // USB LAN
	continue;
	if (USBStorage_Open(&__usbfd, buffer[i].device_id, vid, pid) < 0)
	continue;
	maxLun = USBStorage_GetMaxLUN(&__usbfd);
	for (j = 0; j < maxLun; j++) {
	retval = USBStorage_MountLUN(&__usbfd, j);
	if (retval == INVALID_LUN)
	continue;
	if (retval < 0)
	{
	__usbstorage_reset(&__usbfd);
	continue;
	}
	__mounted = true;
	__lun = j;
	__vid = vid;
	__pid = pid;
	usb_last_used = gettime()-secs_to_ticks(100);
	usleep(10000);
	break;
	}
	if (__mounted)
	break;
	USBStorage_Close(&__usbfd);
	}

This behavior affected WiiFlow on Wii U as documented in WiiFlow issue 338. WiiFlow contains its own modified copy of this function and a fix for the issue can be viewed in pull request 343. Opening this issue to track the problem in libogc and to note that the fix in WiiFlow could be adapted and included here if desired.

[DacoTaco]

hey, thanks for the report!

first of all, i edited your report because devkitPro is an organisation, not a tool. so i changed it to libogc.

this is an interesting report though but im not sure the linked fix is a correct fix.
usb_storage should only care about if the usb is accessible, not about what is on it.
the should be the fat/ntfs/... driver that should care if it is an actual filesystem of the given type.

i don't know if doing a fatMount shows this error?

[kcpants]

Thanks for the quick response and for cleaning up my issue description!

I also had a few reservations about the fix in WiiFlow pull request 343 as noted in the WiiFlow issue.

Looking through the functions available in libogc though, I'm beginning to think libogc / libfat may not be built to handle multiple attached usb storage devices. I have not tested fatMount but reading through the code it requires a pointer to a DISC_INTERFACE

https://github.com/devkitPro/libfat/blob/69543f058c10c90cddf3b3243819fa1953e3ea9e/include/fat.h#L81-L89

/*
Mount the device pointed to by interface, and set up a devoptab entry for it as "name:".
You can then access the filesystem using "name:/".
If startSector = 0, it will mount the active partition of the first valid partition on
the disc. Otherwise it will try to mount the partition starting at startSector.
cacheSize specifies the number of pages to allocate for the cache.
This will not startup the disc, so you need to call interface->startup(); first.
/
extern bool fatMount (const char name, const DISC_INTERFACE* interface, sec_t startSector, uint32_t cacheSize, uint32_t SectorsPerPage);

The DISC_INTERFACE corresponding to the usb storage device is obtained by calling the get_io_usbstorage function:

https://github.com/devkitPro/libfat/blob/69543f058c10c90cddf3b3243819fa1953e3ea9e/source/disc.c#L50-L52

static const DISC_INTERFACE* get_io_usbstorage (void) {
return &__io_usbstorage;
}

Which, in turn, appears to reference __io_usbstorage defined in usbstorage.c within libogc. __io_usbstorage is initialized such that it calls the __usbstorage_isInserted function to mount the usb device:

libogc/libogc/usbstorage.c

Lines 997 to 1006 in bc4b778

	DISC_INTERFACE __io_usbstorage = {
	DEVICE_TYPE_WII_USB,
	FEATURE_MEDIUM_CANREAD | FEATURE_MEDIUM_CANWRITE | FEATURE_WII_USB,
	__usbstorage_Startup,
	__usbstorage_IsInserted,
	__usbstorage_ReadSectors,
	__usbstorage_WriteSectors,
	__usbstorage_ClearStatus,
	__usbstorage_Shutdown
	};

This seems to imply that whatever device __usbstorage_isInserted mounts is the only device that will be accessible to libfat. In other words, as far as I can tell libogc appears to only have the concept of a single USB storage device and that device is mounted by calling the __usbstorage_isInserted function referenced in this issue.

Additionally (and perhaps even more convincingly), it looks like libfat relies directly on the isInserted function in its implementation of _FAT_disc_isInserted:
https://github.com/devkitPro/libfat/blob/69543f058c10c90cddf3b3243819fa1953e3ea9e/source/disc.h#L44-L50

/*
Check if a disc is inserted
Return true if a disc is inserted and ready, false otherwise
/
static inline bool _FAT_disc_isInserted (const DISC_INTERFACE disc) {
return disc->isInserted();
}

To me, all this taken together suggests that given how the function is used today, its callers expect it to only mount FAT formatted devices. To your point though, it could be argued that all the callers are actually misusing the function since making usbstorage.c aware of partition formats does feel like a "layering" violation given the way the abstractions are setup.

I'm not sure what the ideal solution is but at this point I'm fairly convinced there is a problem with the current code. One, possibly cleaner, approach to solving this might be to introduce a DISC_INTERFACE for every USB port as is done in WiiFlow's alternate usbstorage library (although that might also be tricky to implement since different consoles have different numbers of ports):
https://github.com/Fledge68/WiiFlow_Lite/blob/d77459ad2ddd62ad9b905f65d59f28c0bd801833/source/devicemounter/usbstorage.c#L319-L396

[dborth]

Yeah this isn't a bug in USB storage, there's a handling layer missing. We did this in wiimc - see FindPartitions

You can also mount multiple partitions if you want. Libogc / libfat only provide simplified tools.

https://github.com/dborth/wiimc/blob/master/source/fileop.cpp

[DacoTaco]

holy damn, what an impressive analysis @kcpants !
your conclusion is correct in that the current disc interfaces only check for one device per type, and only has fat support through libfat.

there was an attempt by wintermute to add more drivers to libogc by using the existing drivers and porting them over, and reuse the disc interfaces for device access.

the idea is the device interface does the raw device access, the driver (libfat, ntfs-3g, ... ) do the filesystem interpretations & handles calls done by fopen and such.
therefor i think the device interface should allow multiple devices, but should not have filesystem stuff. this is indeed a violation of the layers and would result in hard to maintain code (or port to something newer and better).

im conflicted though, because fixing this would require to rewrite it all and therefor break current applications...

@dborth : multiple partitions but 1 from each device, if i see that correctly? i think the issue here is having 2 usbdrives in which the first is not FAT32 and the second is

[kcpants]

Thank you! Happy to help the community and very happy to have my comment novellas read and thoughtfully considered!

Overhauling usbstorage.c to support multiple USB devices does seem like the "correct" solution but I agree that it would almost certainly break dependent applications (unless it was built alongside the current code and the existing function was left in place which also seems pretty messy and unmaintainable).

From a practical standpoint, I'm not aware of any real world need to actually access multiple USB storage devices at this time. I think the main problem with the current implementation is that callers need access to the "right" device but they have no control over which device gets mounted (that behavior depends on what order the devices appear in the device buffer). So given two USB drives with different vendor ids and product ids, an application will behave differently depending on which drive is used as the Wii FAT drive and which is used as the Wii U drive. This can put the end user in a difficult position since the "solution" of swapping the contents of the drives is inconvenient / impractical and, even worse, the problem itself is opaque which makes it nearly impossible for users to reach the conclusion that swapping drives would fix their problem unless they read the source code or sift through forum posts.

A "band-aid" type fix might be to introduce some way to reverse the order that the buffer is iterated over but it's not obvious how to do this cleanly without changing the DISC_INTERFACE since FN_MEDIUM_ISINSERTED takes no arguments.

libogc/gc/ogc/disc_io.h

Lines 49 to 65 in bc4b778

	typedef bool (* FN_MEDIUM_STARTUP)(void) ;
	typedef bool (* FN_MEDIUM_ISINSERTED)(void) ;
	typedef bool (* FN_MEDIUM_READSECTORS)(sec_t sector, sec_t numSectors, void* buffer) ;
	typedef bool (* FN_MEDIUM_WRITESECTORS)(sec_t sector, sec_t numSectors, const void* buffer) ;
	typedef bool (* FN_MEDIUM_CLEARSTATUS)(void) ;
	typedef bool (* FN_MEDIUM_SHUTDOWN)(void) ;
	struct DISC_INTERFACE_STRUCT {
	unsigned long ioType ;
	unsigned long features ;
	FN_MEDIUM_STARTUP startup ;
	FN_MEDIUM_ISINSERTED isInserted ;
	FN_MEDIUM_READSECTORS readSectors ;
	FN_MEDIUM_WRITESECTORS writeSectors ;
	FN_MEDIUM_CLEARSTATUS clearStatus ;
	FN_MEDIUM_SHUTDOWN shutdown ;
	} ;

Perhaps some global state which controls the iteration order could be added and applications could be allowed to modify it. Or, to solve the problem for more than two devices, the function could be altered to iterate through all devices once (rather than breaking immediately) and store the indices of all the "mountable" devices somewhere accessible to callers such that they could then set which one they wanted to access on subsequent calls. This seems a little bit fragile but if the vast majority of users only have one attached USB storage device it may be safe enough (since in that case the new global state would not change user visible behavior at all). If this approach were pursued, you may also need to consider edge cases of devices being inserted / removed between invocations and messing up the indices (although I think the Wii U itself throws some errors if its storage device is removed while the console is powered on so it might be fair to assume that devices are not inserted / removed after power-on).

As for other possible solutions, there is already precedent for hardcoded logic that skips devices in __usbstorage_IsInserted similar to my fix in the WiiFlow repo (as shown below) but in this case it's just not possible to know which device to skip without reading its contents.

libogc/libogc/usbstorage.c

Lines 885 to 886 in 46b47a0

	if (vid == 0x0b95 && pid == 0x7720) // USB LAN
	continue;

All these ideas feel pretty ugly. At this point it's probably worth mentioning that I'm not in need of a fix for this issue since I mainly wanted to prevent future WiiFlow users from struggling through the diagnostic challenge that I just went through and my slightly hacky PR was accepted. My goal in filing this issue here is primarily to document the problem so that other applications relying on libogc/usbstorage.c can be made aware of it and experienced devs have the opportunity to think about it and decide on its future.

[DacoTaco]

Hey, sorry my reply took so long. ive been busy with other stuff but did not forget this!

first off all, i understand you don't have a need for a fix @kcpants, and it was merged in wiiflow etc but optimally that copy in wiiflow shouldn't exist at all and (correct) fixes should be done in libogc instead. this would benefit everyone, which is the goal here and which is why i would rather see this fixed somehow.

that said, think the correct fix would be for applications (and libfat/libfilesystem) to pass the requested partition to the disc interface and ask it to find a certain partition. however, this obviously breaks everything so that will have to wait.
what i am thinking off is basically seeing if we can't implement a function/variable in the interface to look for a partition who has a certain type.
for example, libfat would pass on the the disc interface/usbstorage that it is looking for fat (passing on the ID you had defined there). usbstorage would ignore all devices/partitions that don't have the ID that it was given.
by default it would still pick the first partition, but those who want to ignore the first device, or look for a certain partition, would give it the id to look for.

would this work in the case you have in your mind?
i would need to run this by wintermute to see if it could work in the idea he had for the device interfaces, but i think it could work.

[kcpants]

Thanks for staying on this! I appreciate your dedication and agree that a solution in libogc that all apps could take advantage of would be much more helpful than my one-off patch in WiiFlow.

The approach you've outlined seems like it would work for the Wii U use case. There may be some challenges though. It sounds like the ID you're planning to use would come from the switch statement in the PartFromType function.

	case 0x00: return "Unused";
	case 0x01: return "FAT12";
	case 0x04: return "FAT16";
	case 0x05: return "Extended";
	case 0x06: return "FAT16";
	case 0x07: return "NTFS";
	case 0x0b: return "FAT32";
	case 0x0c: return "FAT32";
	case 0x0e: return "FAT16";
	case 0x0f: return "Extended";
	case 0x82: return "LxSWP";
	case 0x83: return "LINUX";
	case 0x8e: return "LxLVM";
	case 0xa8: return "OSX";
	case 0xab: return "OSXBT";
	case 0xaf: return "OSXHF";
	case 0xbf: return "WBFS";
	case 0xe8: return "LUKS";
	default: return "Unknown";

Note that there are multiple bytes which map to "FAT32" (0x0b and 0x0c) meaning you may need the interface to accept an array of IDs. This would be especially true if libfat supports "FAT16" formatted drives today (the difference between FAT16 and FAT32 looks to be pretty minimal so it may already work with FAT16 even if it hasn't consciously been designed with FAT16 support in mind).

The other challenge I can think of would be if you want to handle both MBR and GPT partition tables. To mount a drive that has at least one partition which matches the partition type passed in you need usbstorage.c to read each drive's partition table, pick out the byte or bytes that represent the filesystem type on the partition, and compare it to the IDs passed into the function. MBR supports 4 partitions while GPT supports 128 partitions. The index of the byte they use to represent partition type within the sector is almost certainly different in which case usbstorage.c may first need to identify the partition table type before checking IDs. On top of this, the actual bytes used to represent partition types in GPT also look to differ. For details related to FAT32 on GPT see the Microsoft Basic Data partition Wikipedia article. In particular:

According to Microsoft, the basic data partition is the equivalent to master boot record (MBR) partition types 0x06 (FAT16B), 0x07 (NTFS or exFAT), and 0x0B (FAT32).[2] In practice, it is equivalent to 0x01 (FAT12), 0x04 (FAT16), 0x0C (FAT32 with logical block addressing), and 0x0E (FAT16 with logical block addressing) types as well.

A basic data partition can be formatted with any file system, although most commonly BDPs are formatted with the NTFS, exFAT, or FAT32 file systems. To programmatically determine which file system a BDP contains, Microsoft specifies that one should inspect the BIOS Parameter Block that is contained in the BDP's Volume Boot Record.

This seems pretty interesting but also looks like it could be a lot to take on. If GPT drives are not supported and its safe to assume all attached drives are MBR then I think the loop in my WiiFlow patch could be adapted to work without too many changes.

One final note on MBR / GPT - Wii U apps support USB drives that have been hidden from the Wii U by modifying the MBR signature so if "partition table aware" code were added to usbstorage.c it should treat both 0x55AA and 0x55AB as MBR (see PartitionHandle.h for an example of this).

#define MBR_SIGNATURE 0x55AA
#define EBR_SIGNATURE MBR_SIGNATURE

/* for WiiU modified drives to ignore formatting it (thx jayjay123) */
#define MBR_SIGNATURE_MOD 0x55AB
#define EBR_SIGNATURE_MOD MBR_SIGNATURE_MOD

[kcpants]

@DacoTaco Just wanted to post an update here to mention that a new solution to this problem that does not require reading partition tables has been found (my original fix broke GPT support in WiiFlow so needed to be reverted). Full details can be found in my comment in the WiiFlow issue. To summarize here, the WiiU USB drive appears to be formatted as encrypted GPT without a protective MBR. Info on the protective MBR from Wikipedia:

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.[11]

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

The new solution ignores drives that do not have an MBR signature (or the modified MBR signature altered by UStealth) rather than ignoring drives without a readable partition (the original code likely only worked because the WiiU drive was GPT, if logic to read GPT partition tables were added the WiiU drive would probably present as "readable").

I don't think code that deals with the layout of the partition table (GPT vs MBR) breaks the abstractions in libogc since it doesn't include any filesystem specifics so it might be more palatable to add here in usbstorage.c. Of course, if higher level code needs to have the ability to format blank drives this fix probably shouldn't be integrated. This solution also doesn't act as a stepping stone for other filesystem drivers which likely would still need some logic for reading partition tables similar to the approach you suggested above if they were added down the road.