Hi Ana,
On 2026-10-28T05:50:48, Ana Bobillo <[email protected]> wrote:
> efi_loader: Measure the GPT the partition driver uses
>
> tcg2_measure_gpt_data() measures the GPT of the disk an EFI application
> is started from into PCR 5, so that a remote verifier, or a key sealed
> to PCR 5, can tell whether the partition table is the expected one. It
> reads the primary GPT itself and measures it without any check, while
> the partition driver checks the primary header and entries and, if
> either is not valid, finds the partitions in the backup GPT. PCR 5 then
> holds a GPT the partitions were not found in. Unused entries are not
> measured but are covered by the CRC of the entry array, so a change to
> one of them leaves PCR 5 as it was while the partitions come from the
> backup GPT, whatever that holds: a disk whose partitions were changed
> that way still attests, and unseals, as the original one.
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.
>
> The sizes in the header are not checked either. The entries are read
> into a buffer whose size is num_partition_entries *
> sizeof_partition_entry, worked out in 32 bits, and walked
> num_partition_entries times with sizeof_partition_entry as the stride,
> so a product that wraps, or an entry size smaller than the GUID read
> from each entry, makes the walks read past the end of the buffer.
> [...]
>
> disk/part_efi.c | 21 ++++
> include/part.h | 26 +++++
> lib/efi_loader/efi_tcg2.c | 51 +++++-----
> test/lib/part_efi.c | 42 +++++++-
> test/py/tests/test_efi_tcg2.py | 222
> +++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 334 insertions(+), 28 deletions(-)
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -875,6 +875,27 @@ int gpt_repair_headers(struct blk_desc *desc)
> +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?
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -875,6 +875,27 @@ int gpt_repair_headers(struct blk_desc *desc)
> + /*
> + * The partitions come from the backup GPT, as the primary one is not
> + * valid, or is marked to be ignored: give its header as the primary
> + * header that gpt_repair_headers() makes from a backup one
> + */
> + 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?
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> @@ -1202,40 +1203,36 @@ tcg2_measure_gpt_data(struct udevice *dev,
> + if (part_get_gpt_primary(desc, gpt_h, &entry)) {
> + log_warning("No valid GPT read, not measured\n");
> + ret = EFI_SUCCESS;
> + goto out2;
> + }
It would be worth including the device name in the warning, so the
user can tell which disk it refers to.
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.
Regards,
Simon
