From 4268f3da52838f935e0d506afe0a84bdea6ae38f Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Tue, 20 Sep 2016 13:06:05 -0700 Subject: [PATCH 1/3] gptrepair: fix status checking None of these status bit checks were correct. Fix and simplify. --- grub-core/commands/gptrepair.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/grub-core/commands/gptrepair.c b/grub-core/commands/gptrepair.c index 38392fd8f..66ac3f7c7 100644 --- a/grub-core/commands/gptrepair.c +++ b/grub-core/commands/gptrepair.c @@ -46,8 +46,6 @@ grub_cmd_gptrepair (grub_command_t cmd __attribute__ ((unused)), grub_device_t dev = NULL; grub_gpt_t gpt = NULL; char *dev_name; - grub_uint32_t primary_crc, backup_crc; - enum grub_gpt_status old_status; if (argc != 1 || !grub_strlen(args[0])) return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required"); @@ -67,29 +65,25 @@ grub_cmd_gptrepair (grub_command_t cmd __attribute__ ((unused)), if (!gpt) goto done; - primary_crc = gpt->primary.crc32; - backup_crc = gpt->backup.crc32; - old_status = gpt->status; - - if (grub_gpt_repair (dev->disk, gpt)) - goto done; - - if (primary_crc == gpt->primary.crc32 && - backup_crc == gpt->backup.crc32 && - old_status && gpt->status) + if ((gpt->status & GRUB_GPT_BOTH_VALID) == GRUB_GPT_BOTH_VALID) { grub_printf_ (N_("GPT already valid, %s unmodified.\n"), dev_name); goto done; } + if ((gpt->status & GRUB_GPT_PRIMARY_VALID) != GRUB_GPT_PRIMARY_VALID) + grub_printf_ (N_("Found invalid primary GPT on %s\n"), dev_name); + + if ((gpt->status & GRUB_GPT_BACKUP_VALID) != GRUB_GPT_BACKUP_VALID) + grub_printf_ (N_("Found invalid backup GPT on %s\n"), dev_name); + + if (grub_gpt_repair (dev->disk, gpt)) + goto done; + if (grub_gpt_write (dev->disk, gpt)) goto done; - if (!(old_status & GRUB_GPT_PRIMARY_VALID)) - grub_printf_ (N_("Primary GPT for %s repaired.\n"), dev_name); - - if (!(old_status & GRUB_GPT_BACKUP_VALID)) - grub_printf_ (N_("Backup GPT for %s repaired.\n"), dev_name); + grub_printf_ (N_("Repaired GPT on %s\n"), dev_name); done: if (gpt) From 3dda6a863a140a593e47621bdf7e213fb8f274a9 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Tue, 20 Sep 2016 12:43:01 -0700 Subject: [PATCH 2/3] gpt: use inline functions for checking status bits This should prevent bugs like 6078f836 and 4268f3da. --- grub-core/commands/gptprio.c | 2 +- grub-core/commands/gptrepair.c | 6 +++--- grub-core/lib/gpt.c | 9 +++++++-- include/grub/gpt_partition.h | 35 +++++++++++++++++++++++++++------- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/grub-core/commands/gptprio.c b/grub-core/commands/gptprio.c index 548925a08..25f867a81 100644 --- a/grub-core/commands/gptprio.c +++ b/grub-core/commands/gptprio.c @@ -91,7 +91,7 @@ grub_find_next (const char *disk_name, if (!gpt) goto done; - if ((gpt->status & GRUB_GPT_BOTH_VALID) != GRUB_GPT_BOTH_VALID) + if (!grub_gpt_both_valid(gpt)) if (grub_gpt_repair (dev->disk, gpt)) goto done; diff --git a/grub-core/commands/gptrepair.c b/grub-core/commands/gptrepair.c index 66ac3f7c7..c17c7346c 100644 --- a/grub-core/commands/gptrepair.c +++ b/grub-core/commands/gptrepair.c @@ -65,16 +65,16 @@ grub_cmd_gptrepair (grub_command_t cmd __attribute__ ((unused)), if (!gpt) goto done; - if ((gpt->status & GRUB_GPT_BOTH_VALID) == GRUB_GPT_BOTH_VALID) + if (grub_gpt_both_valid (gpt)) { grub_printf_ (N_("GPT already valid, %s unmodified.\n"), dev_name); goto done; } - if ((gpt->status & GRUB_GPT_PRIMARY_VALID) != GRUB_GPT_PRIMARY_VALID) + if (!grub_gpt_primary_valid (gpt)) grub_printf_ (N_("Found invalid primary GPT on %s\n"), dev_name); - if ((gpt->status & GRUB_GPT_BACKUP_VALID) != GRUB_GPT_BACKUP_VALID) + if (!grub_gpt_backup_valid (gpt)) grub_printf_ (N_("Found invalid backup GPT on %s\n"), dev_name); if (grub_gpt_repair (dev->disk, gpt)) diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index 4c5156368..d52d1223f 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -631,10 +631,15 @@ grub_gpt_repair (grub_disk_t disk, grub_gpt_t gpt) 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_BOTH_VALID; + 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; @@ -696,7 +701,7 @@ grub_gpt_write (grub_disk_t disk, grub_gpt_t gpt) { /* TODO: update/repair protective MBRs too. */ - if ((gpt->status & GRUB_GPT_BOTH_VALID) != GRUB_GPT_BOTH_VALID) + if (!grub_gpt_both_valid (gpt)) return grub_error (GRUB_ERR_BAD_PART_TABLE, "Invalid GPT data"); grub_dprintf ("gpt", "writing primary GPT to %s\n", disk->name); diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h index cc3a201a5..39388ce6e 100644 --- a/include/grub/gpt_partition.h +++ b/include/grub/gpt_partition.h @@ -161,13 +161,6 @@ typedef enum grub_gpt_status GRUB_GPT_BACKUP_ENTRIES_VALID = 0x20, } grub_gpt_status_t; -#define GRUB_GPT_MBR_VALID (GRUB_GPT_PROTECTIVE_MBR|GRUB_GPT_HYBRID_MBR) -#define GRUB_GPT_PRIMARY_VALID \ - (GRUB_GPT_PRIMARY_HEADER_VALID|GRUB_GPT_PRIMARY_ENTRIES_VALID) -#define GRUB_GPT_BACKUP_VALID \ - (GRUB_GPT_BACKUP_HEADER_VALID|GRUB_GPT_BACKUP_ENTRIES_VALID) -#define GRUB_GPT_BOTH_VALID (GRUB_GPT_PRIMARY_VALID|GRUB_GPT_BACKUP_VALID) - /* UEFI requires the entries table to be at least 16384 bytes for a * total of 128 entries given the standard 128 byte entry size. */ #define GRUB_GPT_DEFAULT_ENTRIES_SIZE 16384 @@ -197,6 +190,34 @@ struct grub_gpt }; typedef struct grub_gpt *grub_gpt_t; +/* Helpers for checking the gpt status field. */ +static inline int +grub_gpt_mbr_valid (grub_gpt_t gpt) +{ + return ((gpt->status & GRUB_GPT_PROTECTIVE_MBR) || + (gpt->status & GRUB_GPT_HYBRID_MBR)); +} + +static inline int +grub_gpt_primary_valid (grub_gpt_t gpt) +{ + return ((gpt->status & GRUB_GPT_PRIMARY_HEADER_VALID) && + (gpt->status & GRUB_GPT_PRIMARY_ENTRIES_VALID)); +} + +static inline int +grub_gpt_backup_valid (grub_gpt_t gpt) +{ + return ((gpt->status & GRUB_GPT_BACKUP_HEADER_VALID) && + (gpt->status & GRUB_GPT_BACKUP_ENTRIES_VALID)); +} + +static inline int +grub_gpt_both_valid (grub_gpt_t gpt) +{ + return grub_gpt_primary_valid (gpt) && grub_gpt_backup_valid (gpt); +} + /* Translate GPT sectors to GRUB's 512 byte block addresses. */ static inline grub_disk_addr_t grub_gpt_sector_to_addr (grub_gpt_t gpt, grub_uint64_t sector) From f4e09602dcdf6931b64c59af4a1a450c2d571a97 Mon Sep 17 00:00:00 2001 From: Michael Marineau Date: Tue, 20 Sep 2016 13:40:11 -0700 Subject: [PATCH 3/3] gpt: allow repair function to noop Simplifies usage a little. --- grub-core/commands/gptprio.c | 5 ++--- grub-core/lib/gpt.c | 4 ++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/grub-core/commands/gptprio.c b/grub-core/commands/gptprio.c index 25f867a81..a439552e1 100644 --- a/grub-core/commands/gptprio.c +++ b/grub-core/commands/gptprio.c @@ -91,9 +91,8 @@ grub_find_next (const char *disk_name, if (!gpt) goto done; - if (!grub_gpt_both_valid(gpt)) - if (grub_gpt_repair (dev->disk, gpt)) - goto done; + if (grub_gpt_repair (dev->disk, gpt)) + goto done; for (i = 0; (part = grub_gpt_get_partentry (gpt, i)) != NULL; i++) { diff --git a/grub-core/lib/gpt.c b/grub-core/lib/gpt.c index d52d1223f..c5100de2d 100644 --- a/grub-core/lib/gpt.c +++ b/grub-core/lib/gpt.c @@ -579,6 +579,10 @@ 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; + grub_dprintf ("gpt", "repairing GPT for %s\n", disk->name); if (disk->log_sector_size != gpt->log_sector_size)