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

gpt: point alternate PartitionEntryLBA to alternate GPT table #246

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

tobhe
Copy link
Contributor

@tobhe tobhe commented Aug 4, 2024

Currently both tables point to the primary GPT partition entry array which doesn't make a lot of sense. Pass "primary" to t.partitionArraySector() to get the right address for each of the tables instead.

I found this because I was building images for a Chrombook and noticed that cgpt always reported the secondary GPT header as corrupted.

For comparison here is the cgpt show output without this fix for a minimal ESP example:

       start        size    part  contents
           0           1          PMBR
           1           1          Pri GPT header
                                  Sig: [EFI PART]
                                  Rev: 0x00010000
                                  Size: 92 (blocks)
                                  Header CRC: 0x56d22730 
                                  My LBA: 1
                                  Alternate LBA: 212991
                                  First LBA: 34
                                  Last LBA: 212958
                                  Disk UUID: F4206CB2-52F5-4BEA-B903-62A6CC3DDAFB
                                  Entries LBA: 2
                                  Number of entries: 128
                                  Size of entry: 128
                                  Entries CRC: 0xc5a89a28 
           2          32          Pri GPT table
        2048      200706       1  Label: "EFI System"
                                  Type: EFI System Partition
                                  UUID: C2BDE81C-A933-421E-B854-A86FFD229D8E
           2          32 INVALID  Sec GPT table
           1           1 INVALID  Sec GPT header
                                  Sig: [EFI PART]
                                  Rev: 0x00010000
                                  Size: 92 (blocks)
                                  Header CRC: 0x995343f3 
                                  My LBA: 212991
                                  Alternate LBA: 1
                                  First LBA: 34
                                  Last LBA: 212958
                                  Disk UUID: F4206CB2-52F5-4BEA-B903-62A6CC3DDAFB
                                  Entries LBA: 2
                                  Number of entries: 128
                                  Size of entry: 128
                                  Entries CRC: 0xc5a89a28 INVALID

And in comparison the fixed version with the secondary table pointing to the secondary partition list:

       start        size    part  contents
           0           1          PMBR
           1           1          Pri GPT header
                                  Sig: [EFI PART]
                                  Rev: 0x00010000
                                  Size: 92 (blocks)
                                  Header CRC: 0x2dd2b955 
                                  My LBA: 1
                                  Alternate LBA: 212991
                                  First LBA: 34
                                  Last LBA: 212958
                                  Disk UUID: EF0BACC6-F30A-4455-9E3E-96AE05F78C6A
                                  Entries LBA: 2
                                  Number of entries: 128
                                  Size of entry: 128
                                  Entries CRC: 0x31d5ac19 
           2          32          Pri GPT table
        2048      200706       1  Label: "EFI System"
                                  Type: EFI System Partition
                                  UUID: 22712D7B-7398-4E20-8484-1FF7F73C3233
      212959          32          Sec GPT table
        2048      200706       1  Label: "EFI System"
                                  Type: EFI System Partition
                                  UUID: 22712D7B-7398-4E20-8484-1FF7F73C3233
      212991           1          Sec GPT header
                                  Sig: [EFI PART]
                                  Rev: 0x00010000
                                  Size: 92 (blocks)
                                  Header CRC: 0x76129696 
                                  My LBA: 212991
                                  Alternate LBA: 1
                                  First LBA: 34
                                  Last LBA: 212958
                                  Disk UUID: EF0BACC6-F30A-4455-9E3E-96AE05F78C6A
                                  Entries LBA: 212959
                                  Number of entries: 128
                                  Size of entry: 128
                                  Entries CRC: 0x31d5ac19 

Currently both tables point to the primary GPT partition entry
array which doesn't make a lot of sense. Pass "primary" to
t.partitionArraySector() to get the right address for each of
the tables instead.
@tobhe
Copy link
Contributor Author

tobhe commented Aug 4, 2024

Just noticed that this was recently changes in #181. Unfortunately that PR doesn't have a lot of context so I don't really understand what motivated this.

@deitch
Copy link
Collaborator

deitch commented Aug 13, 2024

This is a good catch, thank you. Unfortunately, with a year plus of time since #181, I do not recall either. 🤦‍♂️

The more I think about it, the more that change doesn't make sense to me. It should be the following:

  • For primary header: starting LBA of partition entries = LBA(primary header) + 1
  • For secondary header: starting LBA of partition entries = LBA(secondary header) - (size of entries table)

The above is what is provided by partitionArraySector(primary bool). So why was it changed? I have no idea.

Can you make the change locally to revert that apparently erroneous #181 and confirm that it works correctly? If so, I will then commit the revert.

@deitch
Copy link
Collaborator

deitch commented Aug 13, 2024

Or does your comment "the fixed version with the secondary table pointing to the secondary partition list" mean, "I tested it with this commit and it now works correctly"?

@tobhe
Copy link
Contributor Author

tobhe commented Aug 13, 2024

I tested with the commit included in this PR which should be equivalent to a revert of #181 and resulted in the second (correct) output of cpgt show I included above.
I didn't test with an actual revert because I only found the responsible commit after I found the fix, but since the fix literally reverses the commit that broke it I think that should be good enough.

@deitch
Copy link
Collaborator

deitch commented Aug 13, 2024

No need for a revert. I like a clear new commit better for things like this. You say it is good, all tests pass, let's merge it in.

@deitch deitch merged commit e27b493 into diskfs:master Aug 13, 2024
20 checks passed
@deitch
Copy link
Collaborator

deitch commented Aug 13, 2024

Do you think cgpt could be used as a standalone test? Create a gpt filesystem in a file, run cgpt on it to validate it?

@tobhe
Copy link
Contributor Author

tobhe commented Aug 13, 2024

Sure, that sounds like a good idea. I did write a small program that does that anyway. Let me see if I can clean it up and hook it up to the test infrastructure.

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.

2 participants