Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DirectoryBlob: Fix partition size mixup for encrypted Wii discs. #10970

Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Aug 10, 2022

I found this while trying to debug https://bugs.dolphin-emu.org/issues/12965, though this does not actually fix that issue.

What's happening here is that the ContentPartition chunks inside DirectoryBlob are designed to look from the outside as if they are encrypted/hashed, but present their filesize without the headers such a partition would actually have. This confuses non-ReadWiiDecrypted() reads near the end of the partition, because the offset with headers gets compared against the size without headers so the read is considered out of bounds and returns a failure.

I also round the size up to a full group because otherwise this read in VolumeWii::EncryptGroup() fails on the last not-full-group-sized chunk of the partition:

if (!blob->ReadWiiDecrypted(offset + block * BLOCK_DATA_SIZE, BLOCK_DATA_SIZE,
unencrypted_data[block].data(), partition_data_offset))
{
return false;
}

I'm not sure if on a real Wii disc a partition is always full group sized; this could be solved in other ways too if needed. Just rounding to block size seems to work fine actually.

This shouldn't affect much in practice (I assume RawDump would produce incorrect results without this PR), but probably doesn't hurt to fix anyway. I've tested this by forcing DirectoryBlobReader::SupportsReadWiiDecrypted() to always return false and then checking if Properties -> Filesystem -> Extract Entire Disc on a DirectoryBlob survives a roundtrip; without this PR, files near the end of the disc end up corrupt, while with this PR it works fine (minus boot.bin and fst.bin which are affected by the difference in how we build the file system compared to whatever tool was used for official discs).

@JosJuice Please review.

@JosJuice
Copy link
Member

I'm not sure if on a real Wii disc a partition is always full group sized; this could be solved in other ways too if needed.

No, they're only rounded up to a whole number of blocks, not a whole number of groups. What's the underlying reason why it doesn't work if we round to blocks?

@AdmiralCurtiss
Copy link
Contributor Author

It's the call to HashGroup() in EncryptGroup() I linked above:

const bool success =
HashGroup(unencrypted_data.data(), unencrypted_hashes.data(), [&](size_t block) {
if (offset + (block + 1) * BLOCK_DATA_SIZE <= partition_data_decrypted_size)
{
if (!blob->ReadWiiDecrypted(offset + block * BLOCK_DATA_SIZE, BLOCK_DATA_SIZE,
unencrypted_data[block].data(), partition_data_offset))
{
return false;
}
}
else
{
unencrypted_data[block].fill(0);
}
return true;
});

HashGroup() calls the given callback BLOCKS_PER_GROUP times, which causes the later reads of ReadWiiDecrypted() to go out of bounds of any partition that the DirectoryBlobReader stores, so it returns false for the blocks after the end of the last partition. This in turn fails the entire EncryptGroup() call which propagates up as a read error.

@JosJuice
Copy link
Member

This code is designed so that if the offset is out of bounds, the check against partition_data_decrypted_size is supposed to stop the read from happening. So this smells like something is wrong with partition_data_decrypted_size.

@AdmiralCurtiss
Copy link
Contributor Author

Hm, I just tried again with just rounding to block size and it seems to work actually. I think I didn't set the decrypted rounded up data size of the partition itself (that partitions[i].partition.SetDataSize(data_size) call) last time I tried this.

@AdmiralCurtiss AdmiralCurtiss merged commit 92c6407 into dolphin-emu:master Aug 12, 2022
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the directoryblob-encrypted branch August 12, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants