diskfs / go-diskfs

MIT License
515 stars 116 forks source link

gpt: point alternate PartitionEntryLBA to alternate GPT table #246

Closed tobhe closed 3 months ago

tobhe commented 3 months ago

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 
tobhe commented 3 months ago

Just noticed that this was recently changes in https://github.com/diskfs/go-diskfs/pull/181. Unfortunately that PR doesn't have a lot of context so I don't really understand what motivated this.

deitch commented 3 months ago

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:

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.