[U-Boot] [PATCH 1/4] part: efi: Fix offset

Both the config option and the DT options specify the offset to set the GPT at in bytes, yet the code treats those values as block numbers.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- disk/part_efi.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 71c3cb3f78d9..75d0a78f0a1f 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -534,6 +534,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, static uint32_t partition_entries_offset(struct blk_desc *dev_desc) { uint32_t offset_blks = 2; + uint32_t __maybe_unused offset_bytes; int __maybe_unused config_offset;
#if defined(CONFIG_EFI_PARTITION_ENTRIES_OFF) @@ -545,8 +546,9 @@ static uint32_t partition_entries_offset(struct blk_desc *dev_desc) * the disk) for the entries can be set in * CONFIG_EFI_PARTITION_ENTRIES_OFF. */ - offset_blks = + offset_bytes = PAD_TO_BLOCKSIZE(CONFIG_EFI_PARTITION_ENTRIES_OFF, dev_desc); + offset_blks = offset_bytes / dev_desc->blksz; #endif
#if defined(CONFIG_OF_CONTROL) @@ -558,8 +560,10 @@ static uint32_t partition_entries_offset(struct blk_desc *dev_desc) config_offset = fdtdec_get_config_int(gd->fdt_blob, "u-boot,efi-partition-entries-offset", -EINVAL); - if (config_offset != -EINVAL) - offset_blks = PAD_TO_BLOCKSIZE(config_offset, dev_desc); + if (config_offset != -EINVAL) { + offset_bytes = PAD_TO_BLOCKSIZE(config_offset, dev_desc); + offset_blks = offset_bytes / dev_desc->blksz; + } #endif
debug("efi: partition entries offset (in blocks): %d\n", offset_blks);

The start variable is only used inside a loop, and is never affected inside it, so it's a purely local variable.
In the same way the partition size is accessed several times, so we can store it in a variable.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- disk/part_efi.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 75d0a78f0a1f..fa95ce12329a 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -432,7 +432,6 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, disk_partition_t *partitions, int parts) { lbaint_t offset = (lbaint_t)le64_to_cpu(gpt_h->first_usable_lba); - lbaint_t start; lbaint_t last_usable_lba = (lbaint_t) le64_to_cpu(gpt_h->last_usable_lba); int i, k; @@ -448,24 +447,27 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
for (i = 0; i < parts; i++) { /* partition starting lba */ - start = partitions[i].start; + lbaint_t start = partitions[i].start; + lbaint_t size = partitions[i].size; + if (start && (start < offset)) { printf("Partition overlap\n"); return -1; } + if (start) { gpt_e[i].starting_lba = cpu_to_le64(start); - offset = start + partitions[i].size; + offset = start + size; } else { gpt_e[i].starting_lba = cpu_to_le64(offset); - offset += partitions[i].size; + offset += size; } if (offset > (last_usable_lba + 1)) { printf("Partitions layout exceds disk size\n"); return -1; } /* partition ending lba */ - if ((i == parts - 1) && (partitions[i].size == 0)) + if ((i == parts - 1) && (size == 0)) /* extend the last partition to maximuim */ gpt_e[i].ending_lba = gpt_h->last_usable_lba; else @@ -525,7 +527,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, debug("%s: name: %s offset[%d]: 0x" LBAF " size[%d]: 0x" LBAF "\n", __func__, partitions[i].name, i, - offset, i, partitions[i].size); + offset, i, size); }
return 0;

On Wed, Aug 23, 2017 at 04:01:31PM +0200, Maxime Ripard wrote:
The start variable is only used inside a loop, and is never affected inside it, so it's a purely local variable.
In the same way the partition size is accessed several times, so we can store it in a variable.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Tom Rini trini@konsulko.com

On Wed, Aug 23, 2017 at 04:01:31PM +0200, Maxime Ripard wrote:
The start variable is only used inside a loop, and is never affected inside it, so it's a purely local variable.
In the same way the partition size is accessed several times, so we can store it in a variable.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!

The gpt_fill_pte will need to access the device block size. Let's pass the device descriptor as an argument.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- disk/part_efi.c | 7 ++++--- include/part.h | 6 ++++-- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index fa95ce12329a..807d01de39d0 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -428,8 +428,9 @@ int write_gpt_table(struct blk_desc *dev_desc, return -1; }
-int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, - disk_partition_t *partitions, int parts) +int gpt_fill_pte(struct blk_desc *dev_desc, + gpt_header *gpt_h, gpt_entry *gpt_e, + disk_partition_t *partitions, int parts) { lbaint_t offset = (lbaint_t)le64_to_cpu(gpt_h->first_usable_lba); lbaint_t last_usable_lba = (lbaint_t) @@ -633,7 +634,7 @@ int gpt_restore(struct blk_desc *dev_desc, char *str_disk_guid, goto err;
/* Generate partition entries */ - ret = gpt_fill_pte(gpt_h, gpt_e, partitions, parts_count); + ret = gpt_fill_pte(dev_desc, gpt_h, gpt_e, partitions, parts_count); if (ret) goto err;
diff --git a/include/part.h b/include/part.h index 0cd803a9334f..0d5c99836b25 100644 --- a/include/part.h +++ b/include/part.h @@ -289,6 +289,7 @@ int write_gpt_table(struct blk_desc *dev_desc, /** * gpt_fill_pte(): Fill the GPT partition table entry * + * @param dev_desc - block device descriptor * @param gpt_h - GPT header representation * @param gpt_e - GPT partition table entries * @param partitions - list of partitions @@ -296,8 +297,9 @@ int write_gpt_table(struct blk_desc *dev_desc, * * @return zero on success */ -int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, - disk_partition_t *partitions, int parts); +int gpt_fill_pte(struct blk_desc *dev_desc, + gpt_header *gpt_h, gpt_entry *gpt_e, + disk_partition_t *partitions, int parts);
/** * gpt_fill_header(): Fill the GPT header

On Wed, Aug 23, 2017 at 04:01:32PM +0200, Maxime Ripard wrote:
The gpt_fill_pte will need to access the device block size. Let's pass the device descriptor as an argument.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Tom Rini trini@konsulko.com

On Wed, Aug 23, 2017 at 04:01:32PM +0200, Maxime Ripard wrote:
The gpt_fill_pte will need to access the device block size. Let's pass the device descriptor as an argument.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!

The current code checks that no partitions overlap with the GPT partition table using the offset of the first LBA usable for that partition.
This works fine, unless you have a partition entry that is further away than it usually is and you want to create partitions in the gap between the GPT header and the GPT partition entries, for example to reflash a bootloader that needs to be set there.
Rework the test to something a bit smarter that checks whether a partition would overlap with either the GPT header or the partition entries, no matter where it is on the disk.
Partitions that do not have a start LBA specified will still start at the first LBA usable set in the GPT header, to avoid weird behaviours.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- disk/part_efi.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 807d01de39d0..2973d52f6abb 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -445,24 +445,38 @@ int gpt_fill_pte(struct blk_desc *dev_desc, char *str_type_guid; unsigned char *bin_type_guid; #endif + size_t hdr_start = gpt_h->my_lba; + size_t hdr_end = hdr_start + 1; + + size_t pte_start = gpt_h->partition_entry_lba; + size_t pte_end = pte_start + + gpt_h->num_partition_entries * gpt_h->sizeof_partition_entry / + dev_desc->blksz;
for (i = 0; i < parts; i++) { /* partition starting lba */ lbaint_t start = partitions[i].start; lbaint_t size = partitions[i].size;
- if (start && (start < offset)) { - printf("Partition overlap\n"); - return -1; - } - if (start) { - gpt_e[i].starting_lba = cpu_to_le64(start); offset = start + size; } else { - gpt_e[i].starting_lba = cpu_to_le64(offset); + start = offset; offset += size; } + + /* + * If our partition overlaps with either the GPT + * header, or the partition entry, reject it. + */ + if (((start <= hdr_end && hdr_start <= (start + size)) || + (start <= pte_end && pte_start <= (start + size)))) { + printf("Partition overlap\n"); + return -1; + } + + gpt_e[i].starting_lba = cpu_to_le64(start); + if (offset > (last_usable_lba + 1)) { printf("Partitions layout exceds disk size\n"); return -1;

On Wed, Aug 23, 2017 at 04:01:33PM +0200, Maxime Ripard wrote:
The current code checks that no partitions overlap with the GPT partition table using the offset of the first LBA usable for that partition.
This works fine, unless you have a partition entry that is further away than it usually is and you want to create partitions in the gap between the GPT header and the GPT partition entries, for example to reflash a bootloader that needs to be set there.
Rework the test to something a bit smarter that checks whether a partition would overlap with either the GPT header or the partition entries, no matter where it is on the disk.
Partitions that do not have a start LBA specified will still start at the first LBA usable set in the GPT header, to avoid weird behaviours.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Tom Rini trini@konsulko.com

On Wed, Aug 23, 2017 at 04:01:33PM +0200, Maxime Ripard wrote:
The current code checks that no partitions overlap with the GPT partition table using the offset of the first LBA usable for that partition.
This works fine, unless you have a partition entry that is further away than it usually is and you want to create partitions in the gap between the GPT header and the GPT partition entries, for example to reflash a bootloader that needs to be set there.
Rework the test to something a bit smarter that checks whether a partition would overlap with either the GPT header or the partition entries, no matter where it is on the disk.
Partitions that do not have a start LBA specified will still start at the first LBA usable set in the GPT header, to avoid weird behaviours.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!

On Wed, Aug 23, 2017 at 04:01:30PM +0200, Maxime Ripard wrote:
Both the config option and the DT options specify the offset to set the GPT at in bytes, yet the code treats those values as block numbers.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Tom Rini trini@konsulko.com

Hi Tom,
On Thu, Aug 24, 2017 at 09:05:06PM -0400, Tom Rini wrote:
On Wed, Aug 23, 2017 at 04:01:30PM +0200, Maxime Ripard wrote:
Both the config option and the DT options specify the offset to set the GPT at in bytes, yet the code treats those values as block numbers.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Tom Rini trini@konsulko.com
I think that patch should be in the next release, as this option is unusable (or at least in the way it's documented) without it.
Maxime

On 23 Aug 2017, at 16:01, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Both the config option and the DT options specify the offset to set the GPT at in bytes, yet the code treats those values as block numbers.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

On Wed, Aug 23, 2017 at 04:01:30PM +0200, Maxime Ripard wrote:
Both the config option and the DT options specify the offset to set the GPT at in bytes, yet the code treats those values as block numbers.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Applied to u-boot/master, thanks!
participants (3)
-
Dr. Philipp Tomsich
-
Maxime Ripard
-
Tom Rini