diff mbox series

disk: gpt: verify alternate LBA points to last usable LBA

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

Commit Message

Stefan Herbrechtsmeier March 8, 2021, 4:07 p.m. UTC 
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>

---

 disk/part_efi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Heinrich Schuchardt March 8, 2021, 5:38 p.m. UTC | #1 
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",
>
Stefan Herbrechtsmeier March 9, 2021, 9:36 a.m. UTC | #2 
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
Stefan Herbrechtsmeier March 9, 2021, 4:24 p.m. UTC | #3 
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
Heinrich Schuchardt March 9, 2021, 5:15 p.m. UTC | #4 
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
Tom Rini April 13, 2021, 2:28 p.m. UTC | #5 
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 mbox series

Patch

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",