From de8d29ef89887a105f0f3285108ae47a83d0e64f Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 13:44:11 -0700 Subject: [PATCH 1/8] gpt: check header and entries status bits together Use the new status function which checks *_HEADER_VALID and *_ENTRIES_VALID bits together. It doesn't make sense for the header and entries bits to mismatch so don't allow for it. --- grub-core/lib/gpt.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index c5100de2d..edbe87df2 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -589,24 +589,20 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "GPT sector size must match disk sector size"); - if (!(gpt->status & GRUB_GPT_PRIMARY_ENTRIES_VALID || - gpt->status & GRUB_GPT_BACKUP_ENTRIES_VALID)) - return grub_error (GRUB_ERR_BUG, "No valid GPT entries"); - - if (gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) + if (grub_gpt_primary_valid (gpt)) { - grub_dprintf ("gpt", "primary GPT header is valid\n"); + grub_dprintf ("gpt", "primary GPT is valid\n"); backup_header = grub_le_to_cpu64 (gpt->primary.alternate_lba); grub_memcpy (&gpt->backup, &gpt->primary, sizeof (gpt->backup)); } - else if (gpt->status & GRUB_GPT_BACKUP_HEADER_VALID) + else if (grub_gpt_backup_valid (gpt)) { - grub_dprintf ("gpt", "backup GPT header is valid\n"); + grub_dprintf ("gpt", "backup GPT is valid\n"); backup_header = grub_le_to_cpu64 (gpt->backup.header_lba); grub_memcpy (&gpt->primary, &gpt->backup, sizeof (gpt->primary)); } else - return grub_error (GRUB_ERR_BUG, "No valid GPT header"); + return grub_error (GRUB_ERR_BUG, "No valid GPT"); /* Relocate backup to end if disk whenever possible. */ if (grub_gpt_disk_size_valid(disk)) From 1f5d29420c2b13406e7df746eb4105289772048c Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 13:52:52 -0700 Subject: [PATCH 2/8] gpt: be more careful about relocating backup header The header was being relocated without checking the new location is actually safe. If the BIOS thinks the disk is smaller than the OS then repair may relocate the header into allocated space, failing the final validation check. So only move it if the disk has grown. Additionally, if the backup is valid then we can assume its current location is good enough and leave it as-is. --- grub-core/lib/gpt.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index edbe87df2..4d646aacc 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -592,7 +592,17 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) if (grub_gpt_primary_valid (gpt)) { grub_dprintf ("gpt", "primary GPT is valid\n"); + + /* Relocate backup to end if disk if the disk has grown. */ backup_header = grub_le_to_cpu64 (gpt->primary.alternate_lba); + if (grub_gpt_disk_size_valid (disk) && + disk->total_sectors - 1 > backup_header) + { + backup_header = disk->total_sectors - 1; + grub_dprintf ("gpt", "backup GPT header relocated to 0x%llx\n", + (unsigned long long) backup_header); + } + grub_memcpy (&gpt->backup, &gpt->primary, sizeof (gpt->backup)); } else if (grub_gpt_backup_valid (gpt)) @@ -604,12 +614,6 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) else return grub_error (GRUB_ERR_BUG, "No valid GPT"); - /* Relocate backup to end if disk whenever possible. */ - if (grub_gpt_disk_size_valid(disk)) - backup_header = disk->total_sectors - 1; - grub_dprintf ("gpt", "backup GPT header will be located at 0x%llx\n", - (unsigned long long) backup_header); - backup_entries = backup_header - grub_gpt_size_to_sectors (gpt, gpt->entries_size); grub_dprintf ("gpt", "backup GPT entries will be located at 0x%llx\n", From 427fdc58e13d308677e431aac4d4c1c6f64e6446 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 14:33:48 -0700 Subject: [PATCH 3/8] gpt: selectively update fields during repair Just a little cleanup/refactor to skip touching data we don't need to. --- grub-core/lib/gpt.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 4d646aacc..1b7e219f6 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -577,8 +577,6 @@ grub_gpt_get_partentry (grub_gpt_t gpt, grub_uint32_t n) grub_err_t grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) { - grub_uint64_t backup_header, backup_entries; - /* Skip if there is nothing to do. */ if (grub_gpt_both_valid (gpt)) return GRUB_ERR_NONE; @@ -591,6 +589,8 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) if (grub_gpt_primary_valid (gpt)) { + grub_uint64_t backup_header; + grub_dprintf ("gpt", "primary GPT is valid\n"); /* Relocate backup to end if disk if the disk has grown. */ @@ -601,32 +601,28 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) backup_header = disk->total_sectors - 1; grub_dprintf ("gpt", "backup GPT header relocated to 0x%llx\n", (unsigned long long) backup_header); + + gpt->primary.alternate_lba = grub_cpu_to_le64 (backup_header); } grub_memcpy (&gpt->backup, &gpt->primary, sizeof (gpt->backup)); + gpt->backup.header_lba = gpt->primary.alternate_lba; + gpt->backup.alternate_lba = gpt->primary.header_lba; + gpt->backup.partitions = grub_cpu_to_le64 (backup_header - + grub_gpt_size_to_sectors (gpt, gpt->entries_size)); } else if (grub_gpt_backup_valid (gpt)) { grub_dprintf ("gpt", "backup GPT is valid\n"); - backup_header = grub_le_to_cpu64 (gpt->backup.header_lba); + grub_memcpy (&gpt->primary, &gpt->backup, sizeof (gpt->primary)); + gpt->primary.header_lba = gpt->backup.alternate_lba; + gpt->primary.alternate_lba = gpt->backup.header_lba; + gpt->primary.partitions = grub_cpu_to_le64_compile_time (2); } else return grub_error (GRUB_ERR_BUG, "No valid GPT"); - backup_entries = backup_header - - grub_gpt_size_to_sectors (gpt, gpt->entries_size); - grub_dprintf ("gpt", "backup GPT entries will be located at 0x%llx\n", - (unsigned long long) backup_entries); - - /* Update/fixup header and partition table locations. */ - gpt->primary.header_lba = grub_cpu_to_le64_compile_time (1); - gpt->primary.alternate_lba = grub_cpu_to_le64 (backup_header); - gpt->primary.partitions = grub_cpu_to_le64_compile_time (2); - gpt->backup.header_lba = gpt->primary.alternate_lba; - gpt->backup.alternate_lba = gpt->primary.header_lba; - gpt->backup.partitions = grub_cpu_to_le64 (backup_entries); - /* Recompute checksums. */ if (grub_gpt_update_checksums (gpt)) return grub_errno; From d2f9096444d50dba8059d58fb6fe29be99256899 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 14:55:19 -0700 Subject: [PATCH 4/8] gpt: always revalidate when recomputing checksums This ensures all code modifying GPT data include the same sanity check that repair does. If revalidation fails the status flags are left in the appropriate state. --- grub-core/lib/gpt.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 1b7e219f6..204e53b04 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -623,23 +623,9 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) else return grub_error (GRUB_ERR_BUG, "No valid GPT"); - /* Recompute checksums. */ if (grub_gpt_update_checksums (gpt)) return grub_errno; - /* Sanity check. */ - if (grub_gpt_check_primary (gpt)) - return grub_error (GRUB_ERR_BUG, "Generated invalid GPT primary header"); - - gpt->status |= (GRUB_GPT_PRIMARY_HEADER_VALID | - GRUB_GPT_PRIMARY_ENTRIES_VALID); - - if (grub_gpt_check_backup (gpt)) - return grub_error (GRUB_ERR_BUG, "Generated invalid GPT backup header"); - - gpt->status |= (GRUB_GPT_BACKUP_HEADER_VALID | - GRUB_GPT_BACKUP_ENTRIES_VALID); - grub_dprintf ("gpt", "repairing GPT for %s successful\n", disk->name); return GRUB_ERR_NONE; @@ -650,6 +636,12 @@ grub_gpt_update_checksums (grub_gpt_t gpt) { grub_uint32_t crc; + /* Clear status bits, require revalidation of everything. */ + gpt->status &= ~(GRUB_GPT_PRIMARY_HEADER_VALID | + GRUB_GPT_PRIMARY_ENTRIES_VALID | + GRUB_GPT_BACKUP_HEADER_VALID | + GRUB_GPT_BACKUP_ENTRIES_VALID); + /* Writing headers larger than our header structure are unsupported. */ gpt->primary.headersize = grub_cpu_to_le32_compile_time (sizeof (gpt->primary)); @@ -663,6 +655,18 @@ grub_gpt_update_checksums (grub_gpt_t gpt) grub_gpt_header_lecrc32 (&gpt->primary.crc32, &gpt->primary); grub_gpt_header_lecrc32 (&gpt->backup.crc32, &gpt->backup); + if (grub_gpt_check_primary (gpt)) + return grub_error (GRUB_ERR_BUG, "Generated invalid GPT primary header"); + + gpt->status |= (GRUB_GPT_PRIMARY_HEADER_VALID | + GRUB_GPT_PRIMARY_ENTRIES_VALID); + + if (grub_gpt_check_backup (gpt)) + return grub_error (GRUB_ERR_BUG, "Generated invalid GPT backup header"); + + gpt->status |= (GRUB_GPT_BACKUP_HEADER_VALID | + GRUB_GPT_BACKUP_ENTRIES_VALID); + return GRUB_ERR_NONE; } From f24685b22ec4a09506507ba1bc01be49e8230b15 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 15:01:09 -0700 Subject: [PATCH 5/8] gpt: include backup-in-sync check in revalidation --- grub-core/lib/gpt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 204e53b04..abc96dee0 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -373,6 +373,11 @@ grub_gpt_check_backup (grub_gpt_t gpt) if (backup <= end) return grub_error (GRUB_ERR_BAD_PART_TABLE, "invalid backup GPT LBA"); + /* If both primary and backup are valid but differ prefer the primary. */ + if ((gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) && + !grub_gpt_headers_equal (gpt)) + return grub_error (GRUB_ERR_BAD_PART_TABLE, "backup GPT out of sync"); + return GRUB_ERR_NONE; } @@ -428,11 +433,6 @@ grub_gpt_read_backup (grub_disk_t disk, grub_gpt_t gpt) if (grub_le_to_cpu64 (gpt->backup.header_lba) != sector) return grub_error (GRUB_ERR_BAD_PART_TABLE, "invalid backup GPT LBA"); - /* If both primary and backup are valid but differ prefer the primary. */ - if ((gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) && - !grub_gpt_headers_equal(gpt)) - return grub_error (GRUB_ERR_BAD_PART_TABLE, "backup GPT of of sync"); - gpt->status |= GRUB_GPT_BACKUP_HEADER_VALID; return GRUB_ERR_NONE; } From 5342b880f42bf6899eff352c83007b8f0e5fe69d Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 15:29:55 -0700 Subject: [PATCH 6/8] gpt: read entries table at the same time as the header I personally think this reads easier. Also has the side effect of directly comparing the primary and backup tables instead of presuming they are equal if the crc32 matches. --- grub-core/lib/gpt.c | 69 +++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index abc96dee0..1910e0a86 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -32,6 +32,11 @@ GRUB_MOD_LICENSE ("GPLv3+"); static grub_uint8_t grub_gpt_magic[] = GRUB_GPT_HEADER_MAGIC; +static grub_err_t +grub_gpt_read_entries (grub_disk_t disk, grub_gpt_t gpt, + struct grub_gpt_header *header, + void **ret_entries, + grub_size_t *ret_entries_size); char * grub_gpt_guid_to_str (grub_gpt_guid_t *guid) @@ -401,12 +406,21 @@ grub_gpt_read_primary (grub_disk_t disk, grub_gpt_t gpt) return grub_errno; gpt->status |= GRUB_GPT_PRIMARY_HEADER_VALID; + + if (grub_gpt_read_entries (disk, gpt, &gpt->primary, + &gpt->entries, &gpt->entries_size)) + return grub_errno; + + gpt->status |= GRUB_GPT_PRIMARY_ENTRIES_VALID; + return GRUB_ERR_NONE; } static grub_err_t grub_gpt_read_backup (grub_disk_t disk, grub_gpt_t gpt) { + void *entries = NULL; + grub_size_t entries_size; grub_uint64_t sector; grub_disk_addr_t addr; @@ -434,12 +448,35 @@ grub_gpt_read_backup (grub_disk_t disk, grub_gpt_t gpt) return grub_error (GRUB_ERR_BAD_PART_TABLE, "invalid backup GPT LBA"); gpt->status |= GRUB_GPT_BACKUP_HEADER_VALID; + + if (grub_gpt_read_entries (disk, gpt, &gpt->backup, + &entries, &entries_size)) + return grub_errno; + + if (gpt->status & GRUB_GPT_PRIMARY_ENTRIES_VALID) + { + if (entries_size != gpt->entries_size || + grub_memcmp (entries, gpt->entries, entries_size) != 0) + return grub_error (GRUB_ERR_BAD_PART_TABLE, "backup GPT out of sync"); + + grub_free (entries); + } + else + { + gpt->entries = entries; + gpt->entries_size = entries_size; + } + + gpt->status |= GRUB_GPT_BACKUP_ENTRIES_VALID; + return GRUB_ERR_NONE; } static grub_err_t grub_gpt_read_entries (grub_disk_t disk, grub_gpt_t gpt, - struct grub_gpt_header *header) + struct grub_gpt_header *header, + void **ret_entries, + grub_size_t *ret_entries_size) { void *entries = NULL; grub_uint32_t count, size, crc; @@ -481,9 +518,8 @@ grub_gpt_read_entries (grub_disk_t disk, grub_gpt_t gpt, goto fail; } - grub_free (gpt->entries); - gpt->entries = entries; - gpt->entries_size = entries_size; + *ret_entries = entries; + *ret_entries_size = entries_size; return GRUB_ERR_NONE; fail: @@ -522,30 +558,7 @@ grub_gpt_read (grub_disk_t disk) grub_gpt_read_backup (disk, gpt); /* If either succeeded clear any possible error from the other. */ - if (gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID || - gpt->status & GRUB_GPT_BACKUP_HEADER_VALID) - grub_errno = GRUB_ERR_NONE; - else - goto fail; - - /* Similarly, favor the value or error from the primary table. */ - if (gpt->status & GRUB_GPT_BACKUP_HEADER_VALID && - !grub_gpt_read_entries (disk, gpt, &gpt->backup)) - { - grub_dprintf ("gpt", "read valid backup GPT from %s\n", disk->name); - gpt->status |= GRUB_GPT_BACKUP_ENTRIES_VALID; - } - - grub_errno = GRUB_ERR_NONE; - if (gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID && - !grub_gpt_read_entries (disk, gpt, &gpt->primary)) - { - grub_dprintf ("gpt", "read valid primary GPT from %s\n", disk->name); - gpt->status |= GRUB_GPT_PRIMARY_ENTRIES_VALID; - } - - if (gpt->status & GRUB_GPT_PRIMARY_ENTRIES_VALID || - gpt->status & GRUB_GPT_BACKUP_ENTRIES_VALID) + if (grub_gpt_primary_valid (gpt) || grub_gpt_backup_valid (gpt)) grub_errno = GRUB_ERR_NONE; else goto fail; From 7cd866bd2d61452c7049b514f1425bce3806f942 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Wed, 21 Sep 2016 16:02:53 -0700 Subject: [PATCH 7/8] gpt: report all revalidation errors Before returning an error that the primary or backup GPT is invalid push the existing error onto the stack so the user will be told what is bad. --- grub-core/lib/gpt.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 1910e0a86..8dd99780c 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -669,13 +669,19 @@ grub_gpt_update_checksums (grub_gpt_t gpt) grub_gpt_header_lecrc32 (&gpt->backup.crc32, &gpt->backup); if (grub_gpt_check_primary (gpt)) - return grub_error (GRUB_ERR_BUG, "Generated invalid GPT primary header"); + { + grub_error_push (); + return grub_error (GRUB_ERR_BUG, "Generated invalid GPT primary header"); + } gpt->status |= (GRUB_GPT_PRIMARY_HEADER_VALID | GRUB_GPT_PRIMARY_ENTRIES_VALID); if (grub_gpt_check_backup (gpt)) - return grub_error (GRUB_ERR_BUG, "Generated invalid GPT backup header"); + { + grub_error_push (); + return grub_error (GRUB_ERR_BUG, "Generated invalid GPT backup header"); + } gpt->status |= (GRUB_GPT_BACKUP_HEADER_VALID | GRUB_GPT_BACKUP_ENTRIES_VALID); From 8f7045ee191c36c545048d32c4f19fa14e31fb55 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Thu, 22 Sep 2016 10:00:27 -0700 Subject: [PATCH 8/8] gpt: rename and update documentation for grub_gpt_update The function now does more than just recompute checksums so give it a more general name to reflect that. --- grub-core/commands/gptprio.c | 2 +- grub-core/lib/gpt.c | 4 ++-- include/grub/gpt_partition.h | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/grub-core/commands/gptprio.c b/grub-core/commands/gptprio.c index a439552e1..4a24fa62d 100644 --- a/grub-core/commands/gptprio.c +++ b/grub-core/commands/gptprio.c @@ -127,7 +127,7 @@ grub_find_next (const char *disk_name, grub_gptprio_set_tries_left (part_found, tries_left - 1); - if (grub_gpt_update_checksums (gpt)) + if (grub_gpt_update (gpt)) goto done; if (grub_gpt_write (dev->disk, gpt)) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 8dd99780c..2fcb22715 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -636,7 +636,7 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) else return grub_error (GRUB_ERR_BUG, "No valid GPT"); - if (grub_gpt_update_checksums (gpt)) + if (grub_gpt_update (gpt)) return grub_errno; grub_dprintf ("gpt", "repairing GPT for %s successful\n", disk->name); @@ -645,7 +645,7 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) } grub_err_t -grub_gpt_update_checksums (grub_gpt_t gpt) +grub_gpt_update (grub_gpt_t gpt) { grub_uint32_t crc; diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h index ee435d73b..4730fe362 100644 --- a/include/grub/gpt_partition.h +++ b/include/grub/gpt_partition.h @@ -232,11 +232,12 @@ grub_gpt_t grub_gpt_read (grub_disk_t disk); struct grub_gpt_partentry * grub_gpt_get_partentry (grub_gpt_t gpt, grub_uint32_t n); -/* Sync up primary and backup headers, recompute checksums. */ +/* Sync and update primary and backup headers if either are invalid. */ grub_err_t grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt); -/* Recompute checksums, must be called after modifying GPT data. */ -grub_err_t grub_gpt_update_checksums (grub_gpt_t gpt); +/* Recompute checksums and revalidate everything, must be called after + * modifying any GPT data. */ +grub_err_t grub_gpt_update (grub_gpt_t gpt); /* Write headers and entry tables back to disk. */ grub_err_t grub_gpt_write (grub_disk_t disk, grub_gpt_t gpt);