LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] efi: take size of partition entry from GPT header
@ 2018-09-11 16:15 Eugene Korenevsky
  2018-09-11 16:27 ` Davidlohr Bueso
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Eugene Korenevsky @ 2018-09-11 16:15 UTC (permalink / raw)
  To: Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
@ 2018-09-11 16:27 ` Davidlohr Bueso
  2018-09-11 16:42   ` Eugene Korenevsky
  2018-09-11 22:56 ` kbuild test robot
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2018-09-11 16:27 UTC (permalink / raw)
  To: Eugene Korenevsky, linux-efi, linux-kernel, Ard Biesheuvel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:27 ` Davidlohr Bueso
@ 2018-09-11 16:42   ` Eugene Korenevsky
  0 siblings, 0 replies; 13+ messages in thread
From: Eugene Korenevsky @ 2018-09-11 16:42 UTC (permalink / raw)
  To: dave; +Cc: linux-efi, linux-kernel, ard.biesheuvel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
  2018-09-11 16:27 ` Davidlohr Bueso
@ 2018-09-11 22:56 ` kbuild test robot
  2018-09-11 22:59 ` Ard Biesheuvel
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-09-11 22:56 UTC (permalink / raw)
  To: Eugene Korenevsky
  Cc: kbuild-all, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
  2018-09-11 16:27 ` Davidlohr Bueso
  2018-09-11 22:56 ` kbuild test robot
@ 2018-09-11 22:59 ` Ard Biesheuvel
  2018-09-11 23:20 ` kbuild test robot
  2018-09-12  8:38 ` Karel Zak
  4 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2018-09-11 22:59 UTC (permalink / raw)
  To: Eugene Korenevsky, Davidlohr Bueso, linux-efi,
	Linux Kernel Mailing List, 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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
                   ` (2 preceding siblings ...)
  2018-09-11 22:59 ` Ard Biesheuvel
@ 2018-09-11 23:20 ` kbuild test robot
  2018-09-12  8:38 ` Karel Zak
  4 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-09-11 23:20 UTC (permalink / raw)
  To: Eugene Korenevsky
  Cc: kbuild-all, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
                   ` (3 preceding siblings ...)
  2018-09-11 23:20 ` kbuild test robot
@ 2018-09-12  8:38 ` Karel Zak
  2018-09-13 13:06   ` David Laight
  4 siblings, 1 reply; 13+ messages in thread
From: Karel Zak @ 2018-09-12  8:38 UTC (permalink / raw)
  To: Eugene Korenevsky, Davidlohr Bueso, linux-efi, linux-kernel,
	Ard Biesheuvel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-12  8:38 ` Karel Zak
@ 2018-09-13 13:06   ` David Laight
  2018-09-13 19:48     ` Eugene Korenevsky
  0 siblings, 1 reply; 13+ messages in thread
From: David Laight @ 2018-09-13 13:06 UTC (permalink / raw)
  To: Karel Zak, Eugene Korenevsky, Davidlohr Bueso, linux-efi,
	linux-kernel, Ard Biesheuvel

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)


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-13 13:06   ` David Laight
@ 2018-09-13 19:48     ` Eugene Korenevsky
  2018-09-14  9:01       ` David Laight
  0 siblings, 1 reply; 13+ messages in thread
From: Eugene Korenevsky @ 2018-09-13 19:48 UTC (permalink / raw)
  To: David.Laight
  Cc: kzak, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-13 19:48     ` Eugene Korenevsky
@ 2018-09-14  9:01       ` David Laight
  2018-09-14 11:07         ` kzak
  0 siblings, 1 reply; 13+ messages in thread
From: David Laight @ 2018-09-14  9:01 UTC (permalink / raw)
  To: Eugene Korenevsky
  Cc: kzak, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

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)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-14  9:01       ` David Laight
@ 2018-09-14 11:07         ` kzak
  2018-10-06 18:41           ` Eugene Korenevsky
  0 siblings, 1 reply; 13+ messages in thread
From: kzak @ 2018-09-14 11:07 UTC (permalink / raw)
  To: David Laight
  Cc: Eugene Korenevsky, Davidlohr Bueso, linux-efi, linux-kernel,
	Ard Biesheuvel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-09-14 11:07         ` kzak
@ 2018-10-06 18:41           ` Eugene Korenevsky
  2018-10-08 11:15             ` Karel Zak
  0 siblings, 1 reply; 13+ messages in thread
From: Eugene Korenevsky @ 2018-10-06 18:41 UTC (permalink / raw)
  To: kzak
  Cc: David.Laight, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] efi: take size of partition entry from GPT header
  2018-10-06 18:41           ` Eugene Korenevsky
@ 2018-10-08 11:15             ` Karel Zak
  0 siblings, 0 replies; 13+ messages in thread
From: Karel Zak @ 2018-10-08 11:15 UTC (permalink / raw)
  To: Eugene Korenevsky
  Cc: David.Laight, Davidlohr Bueso, linux-efi, linux-kernel, Ard Biesheuvel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-11 16:15 [PATCH v2] efi: take size of partition entry from GPT header Eugene Korenevsky
2018-09-11 16:27 ` Davidlohr Bueso
2018-09-11 16:42   ` Eugene Korenevsky
2018-09-11 22:56 ` kbuild test robot
2018-09-11 22:59 ` Ard Biesheuvel
2018-09-11 23:20 ` kbuild test robot
2018-09-12  8:38 ` Karel Zak
2018-09-13 13:06   ` David Laight
2018-09-13 19:48     ` Eugene Korenevsky
2018-09-14  9:01       ` David Laight
2018-09-14 11:07         ` kzak
2018-10-06 18:41           ` Eugene Korenevsky
2018-10-08 11:15             ` Karel Zak

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox