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

[Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com>]

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