From e4d25afd1857b0ec1a00bb05374dc49a2f62a5d4 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Tue, 23 Aug 2016 13:09:14 -0700 Subject: [PATCH 1/2] gpt: prefer disk size from header over firmware The firmware and the OS may disagree on the disk configuration and size. Although such a setup should be avoided users are unlikely to know about the problem, assuming everything behaves like the OS. Tolerate this as best we can and trust the reported on-disk location over the firmware when looking for the backup GPT. If the location is inaccessible report the error as best we can and move on. --- grub-core/lib/gpt.c | 18 +++++++++++++----- tests/gpt_unit_test.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 2fcb22715..519e05d89 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -425,13 +425,21 @@ grub_gpt_read_backup (grub_disk_t disk, grub_gpt_t gpt) grub_disk_addr_t addr; /* Assumes gpt->log_sector_size == disk->log_sector_size */ - if (grub_gpt_disk_size_valid(disk)) + if (gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) + { + sector = grub_le_to_cpu64 (gpt->primary.alternate_lba); + if (grub_gpt_disk_size_valid (disk) && sector >= disk->total_sectors) + return grub_error (GRUB_ERR_OUT_OF_RANGE, + "backup GPT located at 0x%llx, " + "beyond last disk sector at 0x%llx", + (unsigned long long) sector, + (unsigned long long) disk->total_sectors - 1); + } + else if (grub_gpt_disk_size_valid (disk)) sector = disk->total_sectors - 1; - else if (gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) - sector = grub_le_to_cpu64 (gpt->primary.alternate_lba); else - return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, - "Unable to locate backup GPT"); + return grub_error (GRUB_ERR_OUT_OF_RANGE, + "size of disk unknown, cannot locate backup GPT"); grub_dprintf ("gpt", "reading backup GPT from sector 0x%llx\n", (unsigned long long) sector); diff --git a/tests/gpt_unit_test.c b/tests/gpt_unit_test.c index 9cf3414c2..218b18697 100644 --- a/tests/gpt_unit_test.c +++ b/tests/gpt_unit_test.c @@ -544,6 +544,46 @@ repair_test (void) close_disk (&data); } +/* Finding/reading/writing the backup GPT may be difficult if the OS and + * BIOS report different sizes for the same disk. We need to gracefully + * recognize this and avoid causing trouble for the OS. */ +static void +weird_disk_size_test (void) +{ + struct test_data data; + grub_gpt_t gpt; + + open_disk (&data); + + /* Chop off 65536 bytes (128 512B sectors) which may happen when the + * BIOS thinks you are using a software RAID system that reserves that + * area for metadata when in fact you are not and using the bare disk. */ + grub_test_assert(data.dev->disk->total_sectors == DISK_SECTORS, + "unexpected disk size: 0x%llx", + (unsigned long long) data.dev->disk->total_sectors); + data.dev->disk->total_sectors -= 128; + + gpt = read_disk (&data); + assert_error_stack_empty (); + /* Reading the alternate_lba should have been blocked and reading + * the (new) end of disk should have found no useful data. */ + grub_test_assert ((gpt->status & GRUB_GPT_BACKUP_HEADER_VALID) == 0, + "unreported missing backup header"); + + /* We should be able to reconstruct the backup header and the location + * of the backup should remain unchanged, trusting the GPT data over + * what the BIOS is telling us. Further changes are left to the OS. */ + grub_gpt_repair (data.dev->disk, gpt); + grub_test_assert (grub_errno == GRUB_ERR_NONE, + "repair failed: %s", grub_errmsg); + grub_test_assert (memcmp (&gpt->primary, &example_primary, + sizeof (gpt->primary)) == 0, + "repair corrupted primary header"); + + grub_gpt_free (gpt); + close_disk (&data); +} + static void iterate_partitions_test (void) { @@ -774,6 +814,7 @@ grub_unit_test_init (void) grub_test_register ("gpt_iterate_partitions_test", iterate_partitions_test); grub_test_register ("gpt_large_partitions_test", large_partitions_test); grub_test_register ("gpt_invalid_partsize_test", invalid_partsize_test); + grub_test_register ("gpt_weird_disk_size_test", weird_disk_size_test); grub_test_register ("gpt_search_part_label_test", search_part_label_test); grub_test_register ("gpt_search_uuid_test", search_part_uuid_test); grub_test_register ("gpt_search_disk_uuid_test", search_disk_uuid_test); @@ -791,6 +832,7 @@ grub_unit_test_fini (void) grub_test_unregister ("gpt_iterate_partitions_test"); grub_test_unregister ("gpt_large_partitions_test"); grub_test_unregister ("gpt_invalid_partsize_test"); + grub_test_unregister ("gpt_weird_disk_size_test"); grub_test_unregister ("gpt_search_part_label_test"); grub_test_unregister ("gpt_search_part_uuid_test"); grub_test_unregister ("gpt_search_disk_uuid_test"); From 44f54cbf43c5926cb9adba932a3035373361b8bb Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Thu, 22 Sep 2016 11:18:42 -0700 Subject: [PATCH 2/2] gpt: write backup GPT first, skip if inaccessible. Writing the primary GPT before the backup may lead to a confusing situation: booting a freshly updated system could consistently fail and next boot will fall back to the old system if writing the primary works but writing the backup fails. If the backup is written first and fails the primary is left in the old state so the next boot will re-try and possibly fail in the exact same way. Making that repeatable should make it easier for users to identify the error. Additionally if the firmware and OS disagree on the disk size, making the backup inaccessible to GRUB, then just skip writing the backup. When this happens the automatic call to `coreos-setgoodroot` after boot will take care of repairing the backup. --- grub-core/lib/gpt.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 519e05d89..95538d5a3 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -730,19 +730,39 @@ grub_gpt_write_table (grub_disk_t disk, grub_gpt_t gpt, grub_err_t grub_gpt_write (grub_disk_t disk, grub_gpt_t gpt) { + grub_uint64_t backup_header; + /* TODO: update/repair protective MBRs too. */ if (!grub_gpt_both_valid (gpt)) return grub_error (GRUB_ERR_BAD_PART_TABLE, "Invalid GPT data"); + /* Write the backup GPT first so if writing fails the update is aborted + * and the primary is left intact. However if the backup location is + * inaccessible we have to just skip and hope for the best, the backup + * will need to be repaired in the OS. */ + backup_header = grub_le_to_cpu64 (gpt->backup.header_lba); + if (grub_gpt_disk_size_valid (disk) && + backup_header >= disk->total_sectors) + { + grub_printf ("warning: backup GPT located at 0x%llx, " + "beyond last disk sector at 0x%llx\n", + (unsigned long long) backup_header, + (unsigned long long) disk->total_sectors - 1); + grub_printf ("warning: only writing primary GPT, " + "the backup GPT must be repaired from the OS\n"); + } + else + { + grub_dprintf ("gpt", "writing backup GPT to %s\n", disk->name); + if (grub_gpt_write_table (disk, gpt, &gpt->backup)) + return grub_errno; + } + grub_dprintf ("gpt", "writing primary GPT to %s\n", disk->name); if (grub_gpt_write_table (disk, gpt, &gpt->primary)) return grub_errno; - grub_dprintf ("gpt", "writing backup GPT to %s\n", disk->name); - if (grub_gpt_write_table (disk, gpt, &gpt->backup)) - return grub_errno; - return GRUB_ERR_NONE; }