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

Add XGD support for Kreon Drives #181

Merged
merged 29 commits into from
Nov 3, 2024
Merged

Add XGD support for Kreon Drives #181

merged 29 commits into from
Nov 3, 2024

Conversation

tbejos
Copy link
Contributor

@tbejos tbejos commented Aug 18, 2024

This adds the ability to dump the security sector from a drive flashed with the Kreon drive, unlock the drive to read the full game partition, and lock the drive again to dump the L1 Video portion.

Security Sector Files:

When dumping an XGD there will now be a .security file which is the security sector as it was read from the drive. The code to process and "clean" the security sector is present but will not be saved as a separate file. More processing of the security sector will occur when the info command is extended with dedicated XGD handling.

Important Information

Linux Fix:

For the vendor specific command to set the lock state of the drive it would normally fit fine into a CDB 6, however a bug was discovered where for opcodes with a group code of 6 or 7 (opcodes 0xC0 - 0xFF) libata in the kernel checks a table which expects the CDB to have a length of 10 and will refuse to send the command to the device. This did not affect drives connected via USB-Attached SCSI, but affected all drives connected via SATA or IDE. Both Windows and Linux work when sending it as a CDB 10 so the change was made to make it that even though not all of the bytes are strictly needed.

Partial Security Sector

If the sequence of getting the security sector fails, so long as it didn't fail on the very first transaction it will proceed to attempt to dump the ISO anyways as the important information should have already been acquired. It will log that it was only a partial dump of the security sector so the SS hash will not be valid but the ISO dump itself should be okay.

Things Missing:

  • No changes were made for the info command. @Deterous has plans to add these changes as a follow on PR. Hashing the cleaned SS should happen there. As currently the raw security sector is being saved (including the fake angles).

Testing Status:

All of the following tests were done by myself, unless otherwise noted, using a TS-H353B (SH-D163B after flashing). My dumps had no errors so I manually manipulated the state file to test refine. I have added this modified state file as <image-name>-refine.state in the zip files.

These were all tested on Windows 11, and Arch Linux

XGD1: (http://redump.org/disc/13538/)

  • DUMP
  • REFINE
  • VERIFY
  • SS Matches

XGD2: (http://redump.org/disc/11809/)

  • DUMP
  • REFINE
  • VERIFY
  • SSv1 Matches

XGD3: (http://redump.org/disc/37091/)

  • DUMP
  • REFINE
  • VERIFY
  • SSv1 Matches (Could not find SSv1 listed for any XGD3 games I own so nothing to compare to)

PS2: (http://redump.org/disc/22865/)

  • DUMP
  • REFINE
  • VERIFY

tbejos and others added 22 commits August 2, 2024 01:16
This allows for dumping Xbox Security Sectors from a Kreon Drive.
Currently there are a couple points I would like to revist:

    - Currently always dumps security sectors when dumping with a
      "KREON V1.00" drive, should only dump when XGD game detected
This adds the command to allow for locking/unlocking the drive in order
to read the disc contents.
With this commit the SHA of my ISO matched DIC on Linux, however it
didn't match what I get from DIC on Windows (which is what is in redump)
Add support for XBox 360
Refactor, move code into main loop, minor improvements
libata checks for minimum CDB size based on opcode. In this case it was
(0xFF >> 5) & 7 which was 7 which libata used to index into an array of
minimum sizes and the minimum in this case was 10. By switching from CDB
6 to CDB 10 we can now send Kreon specific commands to the drive when
connected via SATA on Linux.
Unlock Kreon before dumping starts
Fixes an issue where state was not being updated when we write zeroes to
the iso which would cause issues during refine.

Some refactoring as well
Note: SSV2 fixes have been added, however as 0800 support is not
currently present this code should not be needed at this point
in time.
@tbejos
Copy link
Contributor Author

tbejos commented Aug 18, 2024

Here are the files (non-ISO) from all of my testing. There is a single log file which contains DUMP, VERIFY, and REFINE information. (For the refine logs I copied them over as I tested on a copy of the directory to make sure none of the original files were accidentally disturbed)
xgd1.zip
xgd2.zip
xgd3.zip
ps2.zip

@mirh
Copy link

mirh commented Aug 18, 2024

Does this mean that now Kreon drives are just as good as the official 0800 ones at dumping?

@Deterous
Copy link
Contributor

Deterous commented Aug 18, 2024

Does this mean that now Kreon drives are just as good as the official 0800 ones at dumping?

Kreons have always been able to dump the ISO just fine. It is the security sector for Xbox360 discs that the 0800 drive needed for, and this does not change that. 0800 drive support in Redumper will hopefully come in the future.

This fixes an issue where we modify the PFI in order to make the prints
output the correct values. Now instead of modifying the structure itself
we just store the value and use it for logging.
@mirh
Copy link

mirh commented Aug 18, 2024

Mhh.. then what XGD3 security sector is the cover letter even talking about?

Copy link
Owner

@superg superg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it's good, thank you for working on that!
Please address my feedback and we can get that in.
I spent couple hours on this and tired ATM, I will go over the main dump loop code later on a fresh head, just want to make sure there are no regressions.

scsi/cmd.ixx Outdated Show resolved Hide resolved
scsi/cmd.ixx Show resolved Hide resolved
scsi/cmd.ixx Show resolved Hide resolved
scsi/cmd.ixx Outdated Show resolved Hide resolved
utils/xbox.ixx Outdated Show resolved Hide resolved
dvd/dvd_dump.ixx Show resolved Hide resolved
dvd/dvd_dump.ixx Outdated Show resolved Hide resolved
dvd/dvd_dump.ixx Outdated
LOG("");

std::vector<uint8_t> clean_ss = security_sector;
clean_xbox_security_sector(clean_ss);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of clean_ss here, is it just to make sure we refine the same disc or there is more?
Can't ".raw_ss" be used for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .raw_ss is the security sector as received by the drive, this cleaned version is the one that is used for redump submissions as (I might get some details wrong here) my understanding is the security sector contains disc angle data, and if there was a new manufacturing run of a disc with the same data the security sector would have different angles in it. This cleaned version overwrites that angle data in order to make sure that we can check if the rest of the security sector is the same. @Deterous do you happen to know more details about this?

For the non-dump runs though we can check the .raw_ss if you think that would be better. The goal of doing the cleaned version for the comparison is if you have two of the same disc you would be able to verify that the contents of the disc are the same even if it has different angles in the security sector.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may has misinterpreted this, if you are referring to the variable itself, yes I think we can reuse the security_sector variable and don't need to copy to a new vector

Copy link
Contributor

@Deterous Deterous Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason for clean_ss is both for making sure we refine the same disc as you mentioned, and also for redump. The clean SS is the hash that redump puts in comments, and it will match the output of ss_sector_range (dumpers won't have to run that program anymore).
We had to put it in redumper dump code because we use it for checking same disc is being refined as you mentioned.
The .raw_ss contents differ between dumps of the same disc, the cleaning function is meant to remove the parts that differ.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so we have two things:

  1. raw security sector dump that will have different angle data for the discs with otherwise same dump data
  2. security sector dump that has angle data zeroed and redump is using hash of this file in the submission

Specifically, I don't like outputting both files with otherwise identical content when using redumper dump command. This command should always store only what was received from the drive in the most RAW formats.

Here's what I would like to do here:
(1) should be dumped as .security
(2) should be generated from (1) on refine in memory and compared to the one from disc to ensure we refine the same disc

Hash for the cleaned one should be printed in redumper info command from (1) when it detects xbox dump.

Optionally, (2) can be generated during redumper info using a command line option. We have a similar problem with PSX discs where we want to generate .sbi file for libcrypt games (generated from raw .subcode). I have to think what would be the proper implementation of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to make sure I understand everything correctly:

(1) should be dumped as .security

  • Always save (1) for every dump

(2) should be generated from (1) on refine in memory and compared to the one from disc to ensure we refine the same disc

  • When doing a refine create (2) for the existing .security file and for the current disc to be able to do the comparison (but don't save anything to disc, only exists in memory for the given run)

Hash for the cleaned one should be printed in redumper info command from (1) when it detects xbox dump.

  • Create (2) during info to print hash (but don't save as file to disc)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When doing a refine create (2) for the existing .security file and for the current disc to be able to do the comparison (but don't save anything to disc, only exists in memory for the given run)

Yes, basically load .security file to memory, clear angle data, read security sector from the disc, clear it's angle data and compare the two.

Create (2) during info to print hash (but don't save as file to disc)

That should be the output of redumper info command that analyzes dump files and do not access the drive at all. If xbox dump is detected, read .security file, clear angle data in memory, print hash and that's it.

That should come as a separate PR after we're done with this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay great, that is pretty straight forward. Looking at the code I have one more question.

Currently only one user has reported this happening (so I think it should be uncommon) but their drive sometimes fails to send the full SS sequence leading to only a partial SS being received, in the current code I save it as the .security and just log that it is incomplete but didn't perform the cleanup. Now that we are only outputting the .security file should I:

a) Continue to save it anyways
b) Save it but change the extension to something like .partial_security
c) Do NOT save it (should we attempt to continue the dump (theoretically should be fine) or force a failure)

note in all of these cases it would log that it was incomplete

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer (a). We can't always troubleshoot drive reliability so redumper dump does best effort. Warning about it being incomplete is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the lack of response the past few months, I got caught up with some stuff and then never revisited this.

I believe I have updated the behavior to address everything so now it will:

  • Save "raw" SS as .security file
  • Clean SS to continue using but do not save to file
  • When refining (and not forcing) read saved .security and clean it so we can do a proper compare without worrying about faked angles

I re-tested with an XGD1 and XGD3 disc and both dumped correctly. I deleted some random portion of .state for the XGD1 after it completed and did a refine and it completed and the SHA matched (did a manual SHA-1 compute of the ISO after refine)

dvd/dvd_dump.ixx Outdated Show resolved Hide resolved
dvd/dvd_dump.ixx Outdated Show resolved Hide resolved
tbejos added 2 commits August 18, 2024 16:37
- switch to CDB10 Struct fro KREON Lock command
- switch from post- to pre-increment
- switch from std::array<uint32_t, 2> to std::pair<uint32_t, uint32_t>
- remove unneeded clean_ss variable
- switch to using endian_swap instead of manually creating xgd_type data
@Deterous
Copy link
Contributor

This is the general layout (that I'm able to figure out anyway) for the XGD1 and XGD2 Security Sectors (XGD3 is quite different).
However, the only parts that we touch are within security_ranges (for xbox_skip_ranges) and cr_data (for clean_xbox_security_sector) which have their own structure.
I don't think there is any gain in making the SS structured. Rather, it should be treated as a LayerDescriptor at all times, and the specific parts of the media_specific array be accessed.

export struct XGD1_SecuritySector
{
    uint8_t reserved1[703];
    uint32_t cpr_mai;
    uint8_t reserved2[44];
    uint8_t ccrt_version;
    uint8_t ccrt_length;
    uint8_t ccrt[285];
    uint64_t creation_filetime;
    uint8_t unknown2[16];
    uint32_t reserved3;
    uint8_t unknown3[16];
    uint8_t reserved4[84];
    uint64_t authoring_filetime;
    uint32_t cert_time_t;
    uint8_t reserved5[15];
    uint8_t typeA;
    uint8_t game_id[16];
    uint8_t sha1A[20];
    uint8_t signatureA[256];
    uint64_t mastering_filetime;
    uint8_t reserved6[19];
    uint8_t typeB;
    uint8_t factory_id[16];
    uint8_t sha1B[20];
    uint8_t signatureB[64];
    uint8_t ss_version;
    uint8_t ranges_length;
    uint8_t security_ranges[207];
    uint8_t security_ranges_duplicate[207];
    uint8_t reserved7;
};

export struct XGD2_SecuritySector
{
    uint8_t reserved1[239];
    uint64_t unknown1;
    uint8_t sha1X[20];
    uint8_t reserved2[228];
    uint8_t cr_data[207];
    uint8_t reserved3;
    uint32_t cpr_mai;
    uint8_t reserved4[44];
    uint8_t ccrt_version;
    uint8_t ccrt_length;
    uint8_t reserved5[2];
    uint8_t ccrt[252];
    uint8_t reserved6[96];
    uint8_t media_id[16];
    uint8_t reserved7[46];
    uint8_t unknown2;
    uint64_t authoring_filetime;
    uint8_t reserved8[19];
    uint8_t typeA;
    uint8_t game_id[16];
    uint8_t sha1A[20];
    uint8_t signatureA[256];
    uint64_t mastering_filetime;
    uint8_t reserved9[19];
    uint8_t typeB;
    uint8_t factory_id[16];
    uint8_t sha1B[20];
    uint8_t signatureB[64];
    uint8_t ss_version;
    uint8_t ranges_length;
    uint8_t security_ranges[207];
    uint8_t security_ranges_duplicate[207];
    uint8_t reserved10;
};

This pulls the loop out of cmd.ixx when dumping the security sector from
an XGD
@superg
Copy link
Owner

superg commented Aug 30, 2024

@Deterous the structures are useful, thanks!
Even if we don't use it today, can you please add them to xbox.ixx for later?

@tbejos
Copy link
Contributor Author

tbejos commented Aug 30, 2024

Sure I will add the structures for potential future use

This updates the behavior so that while we clean the security sectors
for use they are not saved as a file. When refining we clean the loaded
SS in order to make sure it matches (unless forcing).
Adds currently known fields for XGD1 and XGD2 security sectors for
potential future use.
@tbejos
Copy link
Contributor Author

tbejos commented Oct 20, 2024

I see now that my current branch is behind a few commits from upstream, do you have a preference of if I do a merge or rebase to resolve it? Or do you have another preference for how I should update it?

@Deterous
Copy link
Contributor

I see now that my current branch is behind a few commits from upstream, do you have a preference of if I do a merge or rebase to resolve it? Or do you have another preference for how I should update it?

A merge should be fine, that's what I've done on previous PRs

@superg superg merged commit fc82107 into superg:main Nov 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants