Skip to contentSkip to conversation

efi: take size of partition entry from GPT header

PATCH Superseded Eugene Korenevsky 3 participants 4 messages Sep 12, 2018 – Sep 13, 2018

Subsystems (3)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 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.

Changes since v1: refactoring (extract get_gpt_entry function),

                  fix (&ptes[i] -> pte)

Changes since v2: use le32_to_cpu, fix typo, sanity check for
sizeof_partition_entry

---
 block/partitions/efi.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)
block/partitions/efi.c +21 -11
index 39f70d968754..c8ff7860973d 100644
@@ -429,8 +429,8 @@ static int is_gpt_valid(struct parsed_partitions *state, u64 lba,
 		goto fail;
 	}
 	/* Check that sizeof_partition_entry has the correct value */
-	if (le32_to_cpu((*gpt)->sizeof_partition_entry) != sizeof(gpt_entry)) {
-		pr_debug("GUID Partition Entry Size check failed.\n");
+	if (le32_to_cpu((*gpt)->sizeof_partition_entry) < sizeof(gpt_entry)) {
+		pr_debug("GUID Partition Entry Size is too small.\n");
 		goto fail;
 	}
 
@@ -670,6 +670,12 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
         return 0;
 }
 
+static gpt_entry *get_gpt_entry(gpt_header *gpt, gpt_entry *ptes, u32 index)
+{
+	return (gpt_entry *)((u8 *)ptes +
+			le32_to_cpu(gpt->sizeof_partition_entry) * index);
+}
+
 /**
  * efi_partition(struct parsed_partitions *state)
  * @state: disk parsed partitions
@@ -704,32 +710,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;
Signature · 1 line
2.18.0
maintainer
#1 lore raw

block/partitions/efi.c int efi_partition(struct parsed_partitions *state) · line 715

Quoted 11 lines · Eugene Korenevsky

/**
* efi_partition(struct parsed_partitions *state)
* @state: disk parsed partitions

block/partitions/efi.c +3 -1
@@ -704,32 +710,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++) {

Please rearrange this like:

      for (i = 0;  < le32_to_cpu(gpt->num_partition_entries) &&
      	      	      i < state->limit-1; i++)
Quoted 11 lines
block/partitions/efi.c +4 -3
+		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;
 

block/partitions/efi.c int efi_partition(struct parsed_partitions *state) · line 731

Quoted 10 lines
block/partitions/efi.c +3 -2
-		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))

This is ugly. If you are worried about 80-chars, please do:

    	      	 if (!efi_guidcmp(pte->partition_type_guid,
				PARTITION_LINUX_RAID_GUID))

Thanks,
Davidlohr

#2 lore raw

From: Davidlohr Bueso

Quoted 17 lines

Sent: 12 September 2018 19:55

block/partitions/efi.c int efi_partition(struct parsed_partitions *state) · line 715

Quoted 11 lines · Eugene Korenevsky

/**
* efi_partition(struct parsed_partitions *state)
* @state: disk parsed partitions

block/partitions/efi.c +3 -1
@@ -704,32 +710,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++) {

Please rearrange this like:

Quoted 2 lines
       for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) &&
       	      	      i < state->limit-1; i++)

Better still cache le32_to_cpu(gpt->num_partition_entries) to a local so that it only has to be calculated once (even if it gets spilled to the stack).
It can be bounded by state->limit-1 at the same time.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

#3 lore raw
Quoted 9 lines
Quoted 4 lines

Please rearrange this like:

       for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) &&
                            i < state->limit-1; i++)

Better still cache le32_to_cpu(gpt->num_partition_entries) to a local so that it only has to be calculated once (even if it gets spilled to the stack).
It can be bounded by state->limit-1 at the same time.

This is block I/O, slow path. No need for such micro optimizations here. GCC is capable to do such optimizations automatically, and it does as I can see from disassembly.
Anyway this would be a subject for another patch.

Signature · 1 line
Eugene