[PATCH v2] efi: take size of partition entry from GPT header

[PATCH v2] efi: take size of partition entry from GPT header

[Eugene Korenevsky]

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 <ekorenevsky@gmail.com>
---
 block/partitions/efi.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 39f70d968754..724f7c0805a2 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -428,11 +428,6 @@ static int is_gpt_valid(struct parsed_partitions *state, u64 lba,
 			 (unsigned long long)le64_to_cpu((*gpt)->first_usable_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");
-		goto fail;
-	}
 
 	/* 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

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Davidlohr Bueso]

On Tue, 11 Sep 2018, Eugene Korenevsky wrote:

>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.

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

Thanks,
Davidlohr

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Eugene Korenevsky]

> >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)?

A disk with correct large-sized GPT entries can be created. UEFI
firmwares will work with it, Linux kernel will not. Is it necessary to
perform such synthetic test or this issue does not matter anyway?

--
Eugene

Re: [PATCH v2] efi: take size of partition entry from GPT header

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

Hi Eugene,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on v4.19-rc3 next-20180911]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eugene-Korenevsky/efi-take-size-of-partition-entry-from-GPT-header/20180912-054430
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-x018-201836 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   block/partitions/efi.c: In function 'get_gpt_entry':
>> block/partitions/efi.c:670:36: error: 'gpt' undeclared (first use in this function); did you mean 'iput'?
     return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);
                                       ^~~
                                       iput
   block/partitions/efi.c:670:36: note: each undeclared identifier is reported only once for each function it appears in
   block/partitions/efi.c:671:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +670 block/partitions/efi.c

   667	
   668	static gpt_entry *get_gpt_entry(gpt_header *gpt_hdr, gpt_entry *ptes, u32 index)
   669	{
 > 670		return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);
   671	}
   672	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33177 bytes --]

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Ard Biesheuvel]

On 11 September 2018 at 18:15, Eugene Korenevsky <ekorenevsky@gmail.com> wrote:
> 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 <ekorenevsky@gmail.com>
> ---
>  block/partitions/efi.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> index 39f70d968754..724f7c0805a2 100644
> --- a/block/partitions/efi.c
> +++ b/block/partitions/efi.c
> @@ -428,11 +428,6 @@ static int is_gpt_valid(struct parsed_partitions *state, u64 lba,
>                          (unsigned long long)le64_to_cpu((*gpt)->first_usable_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");
> -               goto fail;
> -       }
>

As I asked you before, can we keep this sanity check but change != to < ?

>         /* 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
>

Re: [PATCH v2] efi: take size of partition entry from GPT header

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

Hi Eugene,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on v4.19-rc3 next-20180911]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eugene-Korenevsky/efi-take-size-of-partition-entry-from-GPT-header/20180912-054430
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-s0-201836 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   block//partitions/efi.c: In function 'get_gpt_entry':
>> block//partitions/efi.c:670:36: error: 'gpt' undeclared (first use in this function)
     return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);
                                       ^~~
   block//partitions/efi.c:670:36: note: each undeclared identifier is reported only once for each function it appears in
>> block//partitions/efi.c:671:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +/gpt +670 block//partitions/efi.c

   667	
   668	static gpt_entry *get_gpt_entry(gpt_header *gpt_hdr, gpt_entry *ptes, u32 index)
   669	{
 > 670		return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);
 > 671	}
   672	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28782 bytes --]

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Karel Zak]

On Tue, Sep 11, 2018 at 07:15:27PM +0300, Eugene Korenevsky wrote:
> +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)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

RE: [PATCH v2] efi: take size of partition entry from GPT header

[David Laight]

From: Karel Zak
> Sent: 12 September 2018 09:39
> On Tue, Sep 11, 2018 at 07:15:27PM +0300, Eugene Korenevsky wrote:
> > +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.

In principle slightly short lengths presumably imply that the disk
was formatted with an older standard - so the last fields should be
ignored.
They may not be any such disks - until the on-disk structure is extended
and the kernel structure updated to match.

	David

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

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Eugene Korenevsky]

> 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?

-- 
Eugene

RE: [PATCH v2] efi: take size of partition entry from GPT header

[David Laight]

From: Eugene Korenevsky
> Sent: 13 September 2018 20:48
> 
> > 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?

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?

	David

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

Re: [PATCH v2] efi: take size of partition entry from GPT header

[kzak]

On Fri, Sep 14, 2018 at 09:01:48AM +0000, David Laight wrote:
> From: Eugene Korenevsky
> > Sent: 13 September 2018 20:48
> > 
> > > 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?
> 
> 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)

Note that is_gpt_valid() already compares the another LBAs with the 
device size to be sure GPT is no out of reality...

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Eugene Korenevsky]

> 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)
>

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?

--
Eugene

Re: [PATCH v2] efi: take size of partition entry from GPT header

[Karel Zak]

On Sat, Oct 06, 2018 at 09:41:27PM +0300, Eugene Korenevsky wrote:
> > 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)
> >
> 
> Well, we should apply several checks for different cases:
> - primary GPT: table entries should not override gpt->first_usable_lba

 and gpt->last_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?

Yes, the table defines range for all partitions (last and first usable
LBA). All partition table stuff (label and partitions array) has to be
outside this area and partitions have to point to this area.

                   
 | label | entries |    partitioned area   | backup-entries | backup-label |
 
                   ^                       ^
             first_usable_lba        last_usable_lba


and it's possible and valid if there is gap between entries array and 
first usable LBA (you can use this unused place to hide same data :-) 
And vice-versa for backup entries and last usable LBA. 

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com