[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[Stefan Herbrechtsmeier]

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(+)

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

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[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."

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

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[Stefan Herbrechtsmeier]

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

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[Stefan Herbrechtsmeier]

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

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[Heinrich Schuchardt]

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

[PATCH] disk: gpt: verify alternate LBA points to last usable LBA

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/2a65c831/attachment.sig>