Message ID | 20210308160712.75779-1-stefan.herbrechtsmeier-oss@weidmueller.com |
---|---|
State | Accepted |
Commit | d46933839f98d8cdb34fc87299b5f2a4ec4bbfec |
Delegated to: | Tom Rini |
Headers | show |
Series | disk: gpt: verify alternate LBA points to last usable LBA | expand |
On 08.03.21 17:07, Stefan Herbrechtsmeier wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > The gpt command require the GPT backup header at the standard location > at the end of the device.Check the alternate LBA value before reading > the GPT backup header from the last usable LBA of the device. If there is a bug in the gpt command, please, fix it instead of introducing constraints that don't exist in the UEFI specification. The UEFI specification has: "The backup GPT Partition Entry Array must be located after the Last Usable LBA and end before the backup GPT Header." And of course the backup GPT header has to reside within the disk size. There may be good reasons not to place the GPT header on the last LBA. E.g. if the last sector is defective. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > --- > > disk/part_efi.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/disk/part_efi.c b/disk/part_efi.c > index e5636ea7e6..0fb7ff0b6b 100644 > --- a/disk/part_efi.c > +++ b/disk/part_efi.c > @@ -692,6 +692,15 @@ int gpt_verify_headers(struct blk_desc *dev_desc, gpt_header *gpt_head, > /* Free pte before allocating again */ > free(*gpt_pte); > > + /* > + * Check that the alternate_lba entry points to the last LBA > + */ > + if (le64_to_cpu(gpt_head->alternate_lba) != (dev_desc->lba - 1)) { This is wrong. You may check: le64_to_cpu(gpt_head->alternate_lba) < dev_desc->lba and AlternateLBA > LastUsableLBA + ceil(NumberOfPartitionEntries * SizeOfPartitionEntry / SizeOfLBA) > + printf("%s: *** ERROR: Misplaced Backup GPT ***\n", > + __func__); > + return -1; > + } > + > if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), This seems to be one of the buggy lines. You must use the alternate_lba field here. find_valid_gpt() should be corrected, too. Best regards Heinrich > gpt_head, gpt_pte) != 1) { > printf("%s: *** ERROR: Invalid Backup GPT ***\n", >
Hi Heinrich, Am 08.03.2021 um 18:38 schrieb Heinrich Schuchardt: > On 08.03.21 17:07, Stefan Herbrechtsmeier wrote: >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> The gpt command require the GPT backup header at the standard location >> at the end of the device.Check the alternate LBA value before reading >> the GPT backup header from the last usable LBA of the device. > > If there is a bug in the gpt command, please, fix it instead of > introducing constraints that don't exist in the UEFI specification. The constraints already exists because the command only supports backup header at the end. At the moment the command blindly check the backup header at the end even if the primary header points to an other position. > The UEFI specification has: > > "The backup GPT Partition Entry Array must be located after the Last > Usable LBA and end before the backup GPT Header." A lot of tools complain a backup head outside of the last LBA and move the header with the next write. > And of course the backup GPT header has to reside within the disk size. > > There may be good reasons not to place the GPT header on the last LBA. > E.g. if the last sector is defective. At the moment the command only supports a backup header at the end but doesn't check if the primary header points to the last LBA. >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> --- >> >> disk/part_efi.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/disk/part_efi.c b/disk/part_efi.c >> index e5636ea7e6..0fb7ff0b6b 100644 >> --- a/disk/part_efi.c >> +++ b/disk/part_efi.c >> @@ -692,6 +692,15 @@ int gpt_verify_headers(struct blk_desc *dev_desc, gpt_header *gpt_head, >> /* Free pte before allocating again */ >> free(*gpt_pte); >> >> + /* >> + * Check that the alternate_lba entry points to the last LBA >> + */ >> + if (le64_to_cpu(gpt_head->alternate_lba) != (dev_desc->lba - 1)) { > > This is wrong. You may check: > > le64_to_cpu(gpt_head->alternate_lba) < dev_desc->lba > > and > > AlternateLBA > LastUsableLBA + > ceil(NumberOfPartitionEntries * SizeOfPartitionEntry / SizeOfLBA) Why you need this checks? Doesn't this belongs to the check of the gpt itself? >> + printf("%s: *** ERROR: Misplaced Backup GPT ***\n", >> + __func__); >> + return -1; >> + } >> + >> if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), > > This seems to be one of the buggy lines. You must use the alternate_lba > field here. > > find_valid_gpt() should be corrected, too. Okay, I will rework the verify command to support backup headers outside of the last LBA. But I need a command to check if the backup header is at the last LBA. Do you have any suggestion? > >> gpt_head, gpt_pte) != 1) { >> printf("%s: *** ERROR: Invalid Backup GPT ***\n", >> Regards Stefan
Hi Heinrich, Am 08.03.2021 um 18:38 schrieb Heinrich Schuchardt: > On 08.03.21 17:07, Stefan Herbrechtsmeier wrote: >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> The gpt command require the GPT backup header at the standard location >> at the end of the device.Check the alternate LBA value before reading >> the GPT backup header from the last usable LBA of the device. > > If there is a bug in the gpt command, please, fix it instead of > introducing constraints that don't exist in the UEFI specification. > > The UEFI specification has: > > "The backup GPT Partition Entry Array must be located after the Last > Usable LBA and end before the backup GPT Header." "If the primary GPT is invalid, the backup GPT is used instead and it is located on the last logical block on the disk." [UEFI specification 2.8, S. 120] "Note that UEFI standard requires the backup header at the end of the device and partitioning tools can automatically relocate the header to follow the standard." [sfdisk man page, --relocate, gpt-bak-mini] What should U-Boot do? I have a patch to use the backup GPT header if only the header itself is valid but I don't know if this behavior is correct. Regards, Stefan
On 09.03.21 17:24, Stefan Herbrechtsmeier wrote: > Hi Heinrich, > > Am 08.03.2021 um 18:38 schrieb Heinrich Schuchardt: >> On 08.03.21 17:07, Stefan Herbrechtsmeier wrote: >>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>> >>> The gpt command require the GPT backup header at the standard location >>> at the end of the device.Check the alternate LBA value before reading >>> the GPT backup header from the last usable LBA of the device. >> >> If there is a bug in the gpt command, please, fix it instead of >> introducing constraints that don't exist in the UEFI specification. >> >> The UEFI specification has: >> >> "The backup GPT Partition Entry Array must be located after the Last >> Usable LBA and end before the backup GPT Header." > > "If the primary GPT is invalid, the backup GPT is used instead and it is > located on the last logical block on the disk." [UEFI specification 2.8, > S. 120] Thank you for pointing me to this sentence which I missed. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > "Note that UEFI standard requires the backup header at the end of the > device and partitioning tools can automatically relocate the header to > follow the standard." [sfdisk man page, --relocate, gpt-bak-mini] > > What should U-Boot do? I have a patch to use the backup GPT header if > only the header itself is valid but I don't know if this behavior is > correct. > > Regards, > Stefan
On Mon, Mar 08, 2021 at 04:07:11PM +0000, Stefan Herbrechtsmeier wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > The gpt command require the GPT backup header at the standard location > at the end of the device. Check the alternate LBA value before reading > the GPT backup header from the last usable LBA of the device. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Applied to u-boot/master, thanks!
diff --git a/disk/part_efi.c b/disk/part_efi.c index e5636ea7e6..0fb7ff0b6b 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -692,6 +692,15 @@ int gpt_verify_headers(struct blk_desc *dev_desc, gpt_header *gpt_head, /* Free pte before allocating again */ free(*gpt_pte); + /* + * Check that the alternate_lba entry points to the last LBA + */ + if (le64_to_cpu(gpt_head->alternate_lba) != (dev_desc->lba - 1)) { + printf("%s: *** ERROR: Misplaced Backup GPT ***\n", + __func__); + return -1; + } + if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), gpt_head, gpt_pte) != 1) { printf("%s: *** ERROR: Invalid Backup GPT ***\n",