Hi Simon,
On Wed, 28 Oct 2026 at 08:57, Simon Glass <[email protected]> wrote:
> The commit message is very long for the change. Please can you trim it
> to the problem (the GPT measured is not the one the partitions came
> from, and its sizes are not checked) and the fix? The EDK2 comparison
> and the test-case details could be a sentence each.
OK, I'll cut it down to that in v3.
>> +int part_get_gpt_primary(struct blk_desc *desc, gpt_header *gpt_h,
>> + gpt_entry **gpt_pte)
>
> The name is a bit confusing, since the GPT may come from the backup.
> Something like part_get_gpt() or gpt_get_used() would fit better, with
> the header comment saying that a backup header is turned into a
> primary one. What do you think?
Yes, part_get_gpt() reads better, and sits well next to
part_get_gpt_pte(). I'll rename it, and say in the header comment when
a backup header is turned into a primary one and when it is not (below).
>> + if (le64_to_cpu(gpt_h->my_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
>> + restore_primary_gpt_header(gpt_h, desc);
>
> This comment does not hold for the ChromeOS 'IGNOREME' case. There
> is_gpt_valid() returns 2, which gpt_repair_headers() treats as valid,
> so it never rebuilds the primary header. The measured header is then
> one that no 'gpt repair' would ever write. Since the primary header is
> deliberately ignored, I wonder if measuring the backup header as it is
> would be more honest. Either way, please can you correct the comment,
> and the matching claim in the commit message?
You're right, I missed that case, and I agree: with IGNOREME the backup
GPT is the one the disk is meant to be used with, and 'gpt repair'
leaves it that way, so the backup header should be measured as it is.
In v3 part_get_gpt() calls is_gpt_valid() on each header itself, in the
same order as find_valid_gpt(), so that it can tell the two cases apart:
- primary GPT not valid: the backup header is turned into the primary
header that gpt_repair_headers() writes from it, so PCR 5 is the same
as after a 'gpt repair', as discussed with Ilias on v1
- primary header marked IGNOREME: the backup header is given as it is
I'll correct the comment and the commit message to match, and add an
IGNOREME case to the tests (lib_test_gpt_sizes and the PCR 5 test).
>> + log_warning("No valid GPT read, not measured\n");
>
> It would be worth including the device name in the warning, so the
> user can tell which disk it refers to.
Will do.
> BTW now that validate_gpt_header() ensures sizeof_partition_entry
> equals sizeof(gpt_entry), the two loops below could just index
> entry[i] directly. They also use num_partition_entries and
> sizeof_partition_entry without le32_to_cpu(), which could be tidied up
> in a follow-up.
Good point. As there will be a v3 anyway, I'll add that as a small
patch of its own after this one, so that this one stays about the fix.
I'll wait a few days for comments on the rest of the series before
sending v3.
Thanks,
Ana
