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(-)
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
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
> >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
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
On 11 September 2018 at 18:15, Eugene Korenevsky <[email protected]> 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 <[email protected]>
> ---
> 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
>
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
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 <[email protected]>
http://karelzak.blogspot.com
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)
> 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
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)
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 <[email protected]>
http://karelzak.blogspot.com
> 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
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 <[email protected]>
http://karelzak.blogspot.com