efi: take size of partition entry from GPT header

Use gpt_header.sizeof_partition_entry instead of sizeof(gpt_entry) for GPT entry size.
According to UEFI 2.7 spec 5.3.1 "GPT overview":, the size of a GUID Partition Entry element is defined in the Size Of Partition Entry field of GPT header. The GPT with entries sized more than sizeof(gpt_entry) is not illegal. OVMF firmware from EDK2 perfectly works with it, see edk2-tianocore source code.

Quoted 3 lines

The GPT with entries sized more than sizeof(gpt_entry) is not illegal. OVMF firmware from EDK2 perfectly works with it, see edk2-tianocore source code.

But _why_ is this needed? Does this firmware need larger sized entries (ie: does not work without it)?

Use gpt_header.sizeof_partition_entry instead of sizeof(gpt_entry) for GPT entry size.
According to UEFI 2.7 spec 5.3.1 "GPT overview":, the size of a GUID Partition Entry element is defined in the Size Of Partition Entry field of GPT header. The GPT with entries sized more than sizeof(gpt_entry) is not illegal. OVMF firmware from EDK2 perfectly works with it, see edk2-tianocore source code.

Changes since v1: refactoring (extract get_gpt_entry function),

                    fix (&ptes[i] -> pte)

Signed-off-by Eugene Korenevsky <[email protected]>

---
 block/partitions/efi.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

block/partitions/efi.c +0 -0

index 39f70d968754..724f7c0805a2 100644

block/partitions/efi.c +18 -10

        /* Sanity check partition table size */
        pt_size = (u64)le32_to_cpu((*gpt)->num_partition_entries) *

@@ -670,6 +665,11 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
         return 0;
 }
 
+static gpt_entry *get_gpt_entry(gpt_header *gpt_hdr, gpt_entry *ptes, u32 index)
+{
+       return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);
+}
+
 /**
  * efi_partition(struct parsed_partitions *state)
  * @state: disk parsed partitions

@@ -704,32 +704,36 @@ int efi_partition(struct parsed_partitions *state)
 
        pr_debug("GUID Partition Table is valid!  Yea!\n");
 
-       for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit-1; i++) {
+       for (i = 0;
+            i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit-1;
+            i++) {
+               gpt_entry *pte = get_gpt_entry(gpt, ptes, i);
                struct partition_meta_info *info;
                unsigned label_count = 0;
                unsigned label_max;
-               u64 start = le64_to_cpu(ptes[i].starting_lba);
-               u64 size = le64_to_cpu(ptes[i].ending_lba) -
-                          le64_to_cpu(ptes[i].starting_lba) + 1ULL;
+               u64 start = le64_to_cpu(pte->starting_lba);
+               u64 size = le64_to_cpu(pte->ending_lba) -
+                          le64_to_cpu(pte->starting_lba) + 1ULL;
 
-               if (!is_pte_valid(&ptes[i], last_lba(state->bdev)))
+               if (!is_pte_valid(pte, last_lba(state->bdev)))
                        continue;
 
                put_partition(state, i+1, start * ssz, size * ssz);
 
                /* If this is a RAID volume, tell md */
-               if (!efi_guidcmp(ptes[i].partition_type_guid, PARTITION_LINUX_RAID_GUID))
+               if (!efi_guidcmp(
+                   pte->partition_type_guid, PARTITION_LINUX_RAID_GUID))
                        state->parts[i + 1].flags = ADDPART_FLAG_RAID;
 
                info = &state->parts[i + 1].info;
-               efi_guid_to_str(&ptes[i].unique_partition_guid, info->uuid);
+               efi_guid_to_str(&pte->unique_partition_guid, info->uuid);
 
                /* Naively convert UTF16-LE to 7 bits. */
                label_max = min(ARRAY_SIZE(info->volname) - 1,
-                               ARRAY_SIZE(ptes[i].partition_name));
+                               ARRAY_SIZE(pte->partition_name));
                info->volname[label_max] = 0;
                while (label_count < label_max) {
-                       u8 c = ptes[i].partition_name[label_count] & 0xff;
+                       u8 c = pte->partition_name[label_count] & 0xff;
                        if (c && !isprint(c))
                                c = '!';
                        info->volname[label_count] = c;
--

2.18.0

Sent: 12 September 2018 09:39

block/partitions/efi.c static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt, · line 670

Quoted 3 lines · Eugene Korenevsky

block/partitions/efi.c +3 -0

+static gpt_entry *get_gpt_entry(gpt_header *gpt_hdr, gpt_entry *ptes, u32 index)
+{
+	return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);

I guess header use LE, so you need:

               le32_to_cpu(gpt_hdr->sizeof_partition_entry)

I suspect you also need a sanity check that the value isn't too small or stupidly large.

Sent: 13 September 2018 20:48

Quoted 2 lines

I suspect you also need a sanity check that the value isn't too small or stupidly large.

What would be the criterion for too large entries?

From: Eugene Korenevsky

Quoted 5 lines

Sent: 13 September 2018 20:48

Quoted 2 lines

I suspect you also need a sanity check that the value isn't too small or stupidly large.

Quoted 1 line

What would be the criterion for too large entries?

Anything larger than the maximum size of the full GPT table would be a start.
Even something like 64k would stop later calculations going wrong. I presume there is a check elsewhere that the GPT table entries are all inside the disk area that was read?

is_gpt_valid() already contains

        pt_size = (u64)le32_to_cpu((*gpt)->num_partition_entries) *
                le32_to_cpu((*gpt)->sizeof_partition_entry);
        if (pt_size > KMALLOC_MAX_SIZE)
                pr_debug("GUID Partition Table is too large: %llu > %lu bytes\n",
                (unsigned long long)pt_size, KMALLOC_MAX_SIZE);
                goto fail;
        }

I guess it good enough for sanity check.

If you want to be really paranoid than you can also check that array is possible to store to the expected area on the disk:

    pt_size <= (gpt->first_usable_lba - gpt->partition_entry_lba)

Quoted 10 lines

is_gpt_valid() already contains

        pt_size = (u64)le32_to_cpu((*gpt)->num_partition_entries) *
                le32_to_cpu((*gpt)->sizeof_partition_entry);
        if (pt_size > KMALLOC_MAX_SIZE)
                pr_debug("GUID Partition Table is too large: %llu > %lu bytes\n",
                (unsigned long long)pt_size, KMALLOC_MAX_SIZE);
                goto fail;
        }

I guess it good enough for sanity check.

Quoted 5 lines

If you want to be really paranoid than you can also check that array is possible to store to the expected area on the disk:

    pt_size <= (gpt->first_usable_lba - gpt->partition_entry_lba)

Well, we should apply several checks for different cases:
- primary GPT: table entries should not override gpt->first_usable_lba

- alternate GPT, table entries BEFORE agpt (agpt->partition_entry_lba < agpt_lba): table entries should not override agpt_lba AND agpt->partition_entry_lba MUST BE more than agpt->last_usable_lba
- alternate GPT, table entries AFTER agpt (agpt->partition_entry_lba > agpt_lba): table entries should not override the end of the disk

Is this correct?