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

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

[Maxime Ripard]

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

[U-Boot] [PATCH 2/4] part: efi: rework the partition start and size in gpt_fill_pte

[Maxime Ripard]

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

[U-Boot] [PATCH 3/4] part: efi: make gpt_fill_pte take the device descriptor

[Maxime Ripard]

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

[U-Boot] [PATCH 4/4] part: efi: Disable overlap check

[Maxime Ripard]

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

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

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170824/79859c50/attachment.sig>

[U-Boot] [PATCH 2/4] part: efi: rework the partition start and size in gpt_fill_pte

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170824/492ad83b/attachment.sig>

[U-Boot] [PATCH 3/4] part: efi: make gpt_fill_pte take the device descriptor

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170824/5fc2b952/attachment.sig>

[U-Boot] [PATCH 4/4] part: efi: Disable overlap check

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170824/263f0e48/attachment.sig>

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

[Dr. Philipp Tomsich]

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

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

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170828/3182dcb2/attachment.sig>

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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170903/f22be3e9/attachment.sig>

[U-Boot] [U-Boot, 2/4] part: efi: rework the partition start and size in gpt_fill_pte

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170903/b9fe2dcd/attachment.sig>

[U-Boot] [U-Boot, 3/4] part: efi: make gpt_fill_pte take the device descriptor

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170903/16317880/attachment.sig>

[U-Boot] [U-Boot,4/4] part: efi: Disable overlap check

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170903/57b3ee8e/attachment.sig>