- Ignore USB storage devices without an MBR or WBFS signature

[kcpants]

• Ignore USB storage devices without an MBR or WBFS signature (fixes #338, reverts #343) thanks to wiidev/blackb0x for error handling and wbfs check

The original fix for issue #338 breaks support for GPT drives. It was subsequently discovered that the WiiU USB storage drive does not have a protective MBR and that, prior to the original fix, WiiFlow only supported reading from GPT drives with a protective MBR. This updated fix ignores all drives without an MBR signature or a WBFS signature, thereby restoring support for GPT drives with a protective MBR. For more information see this comment.

This code has been tested with a GPT formatted USB drive, an MBR formatted USB drive, and a WBFS formatted USB drive.

[Fledge68]

Hey so i talked to blackb0x/wiidev about this and he pointed out you're forgetting about WBFS formatted drives. even though hardly anyone uses them anymore. you might want to change your commit to match his. here's what he said -

That's almost the same as the WIP code that I'm using in USB Loader GX, but his code won't handle WBFS formatted drives correctly. My version does though and it's got more checks in place.

C:

    for (j = 0; j < maxLun; j++) {
        retval = USBStorage_OGC_MountLUN(&__usbfd, j);

        if (retval == INVALID_LUN)
            continue;

        if (retval < 0)
        {
            __usbstorage_ogc_reset(&__usbfd);
            continue;
        }

        u8* mbr = (u8*)__lwp_heap_allocate(&__heap, 512);
        if (mbr)
        {
            USBStorage_OGC_Read(&__usbfd, j, 0, 1, mbr);
            // WBFS, GPT (protective MBR) & MBR will have one of these
            if (*((u32 *) (mbr)) != 0x57424653 && *((u16 *) (mbr + 0x1FE)) != 0x55AA && *((u16 *) (mbr + 0x1FE)) != 0x55AB)
            {
                gprintf("Skipping USB device (vid %lu pid %lu)\n", vid, pid);
                __lwp_heap_free(&__heap, mbr);
                __usbstorage_ogc_reset(&__usbfd);
                continue;
            }
            __lwp_heap_free(&__heap, mbr);
        }

        __mounted = true;
        __lun = j;
        __vid = vid;
        __pid = pid;
        usb_last_used = gettime()-secs_to_ticks(100);
        usleep(10000);
        break;
    }

0x57424653 is the header for "WBFS". notice he doesn't free the heap until he's done with the IF check. and i assume if the heap allocate fails then the check will not be done. if you use this you should give some credit to him. this will be useful for DacoTaco as well.

And he also said - as for this...

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.

That's by design, since the HBC only works correctly with MBR/FAT32. So it'd be pointless to make the loaders to expect anything but that for SD cards.

[kcpants]

Glad that blackb0x/wiidev caught my mistake! I haven't really worked on consoles before and usually code in languages with built-in memory management. Partition table layouts are new to me and I haven't been in the scene long enough to know that entire drives could be formatted as wbfs. I've learned a lot just trying to get this issue fixed without breaking anything!

I made some improvements to my patch based on blackb0x/wiidev's code and will credit them in the commit message.

notice he doesn't free the heap until he's done with the IF check

I think this is only needed in his implementation because his if statement is indexing directly into the heap (via mbr) whereas mine stores the relevant data in local variables on the stack. I like having a single call to __lwp_heap_free at the cost of two locals, especially since the MBR signature needs to be checked twice. I did move the declaration of my locals up to the top of the function to match the style of the existing code.

For future readers, 0x57424653 is the characters "WBFS" in hex. The magic is defined in the libwbfs repo here. I made macros for these constants similar to the ones defined in PartitionHandle.h to improve readability and make the code more accessible for new contributors in the future.

I want to try to test with a WBFS drive just to be certain it works (I'd like to avoid submitting untested code if possible). After that, I'll update this PR.

[wiidev]

I think this is only needed in his implementation because his if statement is indexing directly into the heap (via mbr) whereas mine stores the relevant data in local variables on the stack. I like having a single call to __lwp_heap_free at the cost of two locals, especially since the MBR signature needs to be checked twice. I did move the declaration of my locals up to the top of the function to match the style of the existing code.

You should check the result of lwp_heap_allocate() because it can fail and that's why there's at least 3 similar checks within the file. So if it did fail and you were to call lwp_heap_free() with the mbr pointer being null then I assume it'd crash the application.

I'd actually told Fledge68 a day after he merged your original pull request that it'd break GPT and WBFS support, but at the time I didnt have a Wii U to offer an improved tested solution. A few weeks later I got one though and that's how I ended up with the code that's now shared here, which is of course inspired by your original workaround.

I don't think encrypted Wii U storage devices have any identifiers and instead require you to decrypt them with keys from the OTP and SEEPROM.

[kcpants]

You should check the result of __lwp_heap_allocate() because it can fail and that's why there's at least 3 similar checks within the file.

Yeah, I definitely missed the null check. As I mentioned, most of my recent programming experience is in higher-level garbage collected languages. The observation I was making in the comment you quoted was just that you can free before the if statement if you copy the relevant sections of the mbr sector into local variables (to avoid referencing the heap after freeing). Here's a preview of the main body of my latest version of the fix. The logic matches your solution that fledge68 posted above. I just made a few small stylistic changes to make the code a little more readable for newcomers.

            u8* mbr = (u8*)__lwp_heap_allocate(&__heap, 512);
            USBStorage_OGC_Read(&__usbfd, j, 0, 1, mbr);
            if (mbr)
            {
                mbrSignature = ((u16*)mbr)[255];
                wbfsHeaderMagic = *((u32*)mbr);
                __lwp_heap_free(&__heap, mbr);

                if (mbrSignature != MBR_SIGNATURE && mbrSignature != MBR_SIGNATURE_MOD && wbfsHeaderMagic != WBFS_HEADER_MAGIC)
                {
                    gprintf("USB storage device (vid %lu pid %lu) cannot be identified as MBR, GPT, or WBFS. Skipping...\n", vid, pid);
                    __usbstorage_ogc_reset(&__usbfd);
                    continue;
                }
            }

I don't think encrypted Wii U storage devices have any identifiers and instead require you to decrypt them with keys from the OTP and SEEPROM.

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

I'm not sure what parts of the GPT spec WiiU storage drives adhere to and what they ignore but from my experiments I found that they do have the "GPT magic" used to identify GPT drives found in the CheckGPT function: https://github.com/Fledge68/WiiFlow_Lite/blob/d77459ad2ddd62ad9b905f65d59f28c0bd801833/source/devicemounter/PartitionHandle.cpp#L335-L351 Specifically, the first 8 bytes of sector 1 are "EFI PART". After I discovered this, I inferred that the WiiU uses some form of GPT and then just blanket encrypts the entire drive with the console-specific encryption keys. This would make sense from a development standpoint since it would take a lot more effort to write a custom partition table layout than to write some low-level code that intercepts reads/writes and decrypts/encrypts just before they hit the drive and the latter is much more secure. It appears that this code is so low level that any code running on the console may be able to read the decrypted contents of the WiiU storage drive without explicitly decrypting it.

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

As an aside, I've been trying to test WBFS with the latest version of the code and found that Wii Backup Manager's current implementation of WBFS actually has an MBR signature and doesn't have the WBFS magic. For reference, here's a screenshot of the first sector of a drive I recently formatted as WBFS: Of course, it's possible another tool still uses the magic and people with older setups also might still have drives formatted with the original libwbfs so to test the code I'm going to manually modify the contents of sector 0 to remove the MBR signature and add back the original WBFS magic. Just thought it was interesting that, like all of us, as WBFS aged even it couldn't avoid losing its magic.

[kcpants]

I was able to test the drive skipping logic against my "handmade" old-style WBFS drive and confirmed that it is not skipped. However, neither WiiFlow nor Wii Backup Manager was actually able to read the wbfs files on the drive after I added the magic. This could be because they don't support old-style WBFS drives at all or just that they don't support improperly formatted old-style WBFS drives. According to cyan:

the wbfs partition format has a header (1 sector to list number of games, each byte keep track of one game, so 512byte/sector drive could hold up to 500 games max + 12byte reserved). Then each game had a list of wbfs blocks (1 wbfs block = multiple hdd sectors) assigned to them. the wbfs format was a raw data, without any file system table. Sector 0 was the header, then following sectors are the bloc list and (fragmented) game's data.

Obviously my hand-written drive didn't adhere to the original wbfs format spec described above. Still, it was sufficient for validating that old WBFS drives wont be skipped.

If anyone knows of a tool that creates old-style WBFS partitions I'd be happy to test with it. Regardless, I think this patch is ready to go. Thanks again for catching my mistakes @wiidev! If some version of this code does eventually get ported to libogc it'll probably be really important to include the WBFS check since libogc has a much wider reach and is very likely to interact with legacy applications using WBFS.

[wiidev]

As an aside, I've been trying to test WBFS with the latest version of the code and found that Wii Backup Manager's current implementation of WBFS actually has an MBR signature and doesn't have the WBFS magic.

I blame this on COVID brain fog, since I tested positive for COVID while I was working on this issue and then when I felt better I'd moved onto addressing some other bug reports. So it wasn't until yesterday when Fledge68 mentioned your pull request that I looked at the code again for the first time in weeks.

Give me a little time to investigate this further, since I swear I read somewhere that it's possible to have a storage device formatted to WBFS without an MBR. Maybe it's done with wwt 🤔

[kcpants]

No worries, I hope you're feeling better! I think there's enough evidence that the WBFS magic existed at one point in time that it's probably worth including the check just to be safe, regardless of whether or not modern applications still use it. After all, the fix works with or without the check so there's no harm in keeping it.

In case you want to keep looking, I found a few old videos that use a tool called WBFS manager to format drives as WBFS but folks in gbatemp say it's buggy and should be avoided these days. There's also a list of WBFS tools here. Since I did enough testing to convince myself that the fix with the check you suggested is guaranteed not to skip these drives (even though I couldn't create a real one) I didn't feel motivated to dive down that rabbit hole. Still, I'm happy to test if you do find any modern tools that still have the magic.

[wiidev]

Specifically, the first 8 bytes of sector 1 are "EFI PART". After I discovered this, I inferred that the WiiU uses some form of GPT and then just blanket encrypts the entire drive with the console-specific encryption keys.

My Wii U storage device has no MBR or partition identifiers. It's fully encrypted.

No worries, I hope you're feeling better! I think there's enough evidence that the WBFS magic existed at one point in time that it's probably worth including the check just to be safe, regardless of whether or not modern applications still use it. After all, the fix works with or without the check so there's no harm in keeping it.

I'm doing better now, thanks.

I can see that USB Loader GX does check for this kind of a setup and I found this post by Cyan. So I've shot him a message to find out if he knows how some people were formatting drives to WBFS without an MBR, since it doesn't seem to be the norm.

[kcpants]

My Wii U storage device has no MBR or partition identifiers. It's fully encrypted.

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

Mine is fully encrypted when I try to view its contents outside the WiiU (by plugging the drive into my PC). However, when I added code similar to the following in WiiFlow to test for GPT I found that my WiiU drive was presenting as GPT to WiiFlow:

                        // This exact code block is untested, I'm just paraphrasing what I did when running experiments
            u8* gpt_header = (u8*)__lwp_heap_allocate(&__heap, 512);
            if (gpt_header)
            {
                USBStorage_OGC_Read(&__usbfd, j, 1, 1, gpt_header);
                if (strncmp(((char*) gpt_header), "EFI PART", 8) == 0) {
                    gprintf("GPT drive detected!\n");
                }
                    __lwp_heap_free(&__heap, gpt_header);
            }

As mentioned in my previous comment, the logic above was taken from PartitionHandle::CheckGPT https://github.com/Fledge68/WiiFlow_Lite/blob/d77459ad2ddd62ad9b905f65d59f28c0bd801833/source/devicemounter/PartitionHandle.cpp#L335-L351

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

Also, I noticed the null check is still out of place in the current version of the PR -- definitely don't want to read the first sector of the drive into address 0x00000000 -- I'll post an update momentarily.

[Fledge68]

I'm going to wait till wiidev hears from cyan. although reading that posts sounds like enough proof. so what do we do if someone has a wbfs drive without a mbr? does it still have a wbfs sig?

[kcpants]

If there is such a thing as WBFS format that has neither the "WBFS" magic nor an MBR signature then the current fix will skip it. That would be pretty surprising though since we know modern WBFS has an MBR signature from direct experiments and cyan's post that wiidev linked to seems to describe tools that convert drives from WBFS with the magic header and no partition table to WBFS with an MBR signature and an MBR partition table.

I'm not sure what would motivate a developer to create a third flavor of WBFS (having two different layouts for block 0 for a single drive format already seems like a lot since knowledge of the layout has to be pre-programmed into any tool that reads the drive).

If it turns out there is such a format, it wouldn't be too hard to adapt the fix assuming that whatever signature/magic it uses to identify itself doesn't collide with the WiiU drive's modified GPT format.

[wiidev]

However, when I added code similar to the following in WiiFlow to test for GPT I found that my WiiU drive was presenting as GPT to WiiFlow

It's weird because it seems like yours is being partially decrypted or something, but mines still fully encrytped and all of the sectors match what my PC sees.

u8* header = (u8*)__lwp_heap_allocate(&__heap, 512);
if (header)
{
    USBStorage_OGC_Read(&__usbfd, j, 1, 1, header);
    hexdump(header, 32); // No "EFI PART" to be found
    __lwp_heap_free(&__heap, header);
}

I can keep my Wii U USB HDD connected even without any of these workarounds though, so maybe that's got something to do with this.

so what do we do if someone has a wbfs drive without a mbr? does it still have a wbfs sig?

When there isn't an MBR you'll find WBFS at the start of sector 0. That's why USB Loader GX checks for an MBR and if it doesn't find one it'll check for WBFS and then add a partition entry if the identifier is found.

[kcpants]

It's weird because it seems like yours is being partially decrypted or something, but mines still fully encrytped and all of the sectors match what my PC sees.

Since we were seeing inconsistent behavior I tried to recreate my original result and, unfortunately I wasn't able to :confused:.

When I was running the tests a few days ago I had a GPT drive and a WiiU drive and was swapping them out between reboots of the console so that only one was attached at a time and then reading the aggregated WiiFlow log to see if the drive was being recognized and mounted. Since neither of us can read meaningful data from sector 1, I think the most likely explanation of what happened is that I misread the aggregated log. Sorry for the trouble/confusion :sweat:. I'll add edits to my previous posts so they don't mislead anyone in the future.

I can keep my Wii U USB HDD connected even without any of these workarounds though, so maybe that's got something to do with this.

I think hitting this bug is just a coinflip. If you swap the contents of your WiiU USB drive and your Wii USB drive I suspect you would trigger it.

FYI, I have a white 8GB WiiU just to eliminate any other possible variables.

[Fledge68]

so to clarify this check will work because it check for MBR sig and WBFS which a wii u drive will not have either?

[wiidev]

FYI, I have a white 8GB WiiU just to eliminate any other possible variables.

I've got a 32GB black Wii U.

so to clarify this check will work because it check for MBR sig and WBFS which a wii u drive will not have either?

Correct. A Wii U storage device won't have either signature, so the loader can safely exclude it.

[kcpants]

I'm going to wait till wiidev hears from cyan. although reading that posts sounds like enough proof.

Has anyone heard word from cyan about wbfs? Based on the reasoning in the comments above, this change seems safe to integrate given everything we know, even if cyan doesn't get back to us. Since the original fix breaks GPT support, this should probably be merged before distributing any new releases.

If there is still any uncertainty about old-style wbfs drives I'm happy to test against them if you can point me to a tool that can create one!