[PATCH 1/2 for 3.12-rc6] Revert "partitions/efi: loosen check fot pmbr size in lba"

[PATCH 1/2 for 3.12-rc6] Revert "partitions/efi: loosen check fot pmbr size in lba"

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This reverts commit 6b02fa59a7cf34c548eedee657b07ea6c54d3894.

This commit is fixing a commit which is going to be reverted. So revert this
one too since it becomes unneeded.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 block/partitions/efi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 1eb09ee..1a5ec9a 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -186,7 +186,6 @@ invalid:
  */
 static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
 {
-	uint32_t sz = 0;
 	int i, part = 0, ret = 0; /* invalid by default */
 
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
@@ -217,15 +216,12 @@ check_hybrid:
 	/*
 	 * Protective MBRs take up the lesser of the whole disk
 	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
-	 * Some partitioning programs, nonetheless, choose to set
-	 * the size to the maximum 32-bit limitation, disregarding
-	 * the disk size.
 	 *
 	 * Hybrid MBRs do not necessarily comply with this.
 	 */
 	if (ret == GPT_MBR_PROTECTIVE) {
-		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
-		if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
+		if (le32_to_cpu(mbr->partition_record[part].size_in_lba) !=
+		    min((uint32_t) total_sectors - 1, 0xFFFFFFFF))
 			ret = 0;
 	}
 done:
-- 
1.8.1.4

[PATCH 2/2 for 3.12-rc6] Revert "partitions/efi: account for pmbr size in lba"

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This reverts commit 27a7c642174eaec627f6a3a254035bf8abd02c5e.

This patch breaks existing systems, for example, Tizen IVI images do not boot
the way they have always been booted because of this patch.

Here is the (pretty standard) use-case:

1. We create a 4GiB image with GPT partition. At this point the protective MBR
   partition contains correct 'size_in_lba'.
2. We dd this image to an 8GiB USB stick and try to boot off this USB stick.

Expected outcome:
  The kernel complains that the alternate GPT header is not at the end of the
  disk, but accepts/parses it anyway and the system boots.

With this patch:
  The kernel just does not recognize the partition table and the system fails
  to boot.

Ther root-cause is the size check introduced in the commit which is being
reverted. That commit does not explain which problem it solves, so I guess this
is more about "hardening" the GPT parser, which does not sound like a good
reason for breaking existing setups, so I think it is best to just revert this
change.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Tested-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 block/partitions/efi.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 1a5ec9a..f5716b8 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -169,7 +169,6 @@ invalid:
 /**
  * is_pmbr_valid(): test Protective MBR for validity
  * @mbr: pointer to a legacy mbr structure
- * @total_sectors: amount of sectors in the device
  *
  * Description: Checks for a valid protective or hybrid
  * master boot record (MBR). The validity of a pMBR depends
@@ -184,9 +183,9 @@ invalid:
  * Returns 0 upon invalid MBR, or GPT_MBR_PROTECTIVE or
  * GPT_MBR_HYBRID depending on the device layout.
  */
-static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
+static int is_pmbr_valid(legacy_mbr *mbr)
 {
-	int i, part = 0, ret = 0; /* invalid by default */
+	int i, ret = 0; /* invalid by default */
 
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
 		goto done;
@@ -194,7 +193,6 @@ static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
 	for (i = 0; i < 4; i++) {
 		ret = pmbr_part_valid(&mbr->partition_record[i]);
 		if (ret == GPT_MBR_PROTECTIVE) {
-			part = i;
 			/*
 			 * Ok, we at least know that there's a protective MBR,
 			 * now check if there are other partition types for
@@ -212,18 +210,6 @@ check_hybrid:
 			EFI_PMBR_OSTYPE_EFI_GPT) &&
 		    (mbr->partition_record[i].os_type != 0x00))
 			ret = GPT_MBR_HYBRID;
-
-	/*
-	 * Protective MBRs take up the lesser of the whole disk
-	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
-	 *
-	 * Hybrid MBRs do not necessarily comply with this.
-	 */
-	if (ret == GPT_MBR_PROTECTIVE) {
-		if (le32_to_cpu(mbr->partition_record[part].size_in_lba) !=
-		    min((uint32_t) total_sectors - 1, 0xFFFFFFFF))
-			ret = 0;
-	}
 done:
 	return ret;
 }
@@ -582,7 +568,6 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 	gpt_header *pgpt = NULL, *agpt = NULL;
 	gpt_entry *pptes = NULL, *aptes = NULL;
 	legacy_mbr *legacymbr;
-	sector_t total_sectors = i_size_read(state->bdev->bd_inode) >> 9;
 	u64 lastlba;
 
 	if (!ptes)
@@ -596,7 +581,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 			goto fail;
 
 		read_lba(state, 0, (u8 *)legacymbr, sizeof(*legacymbr));
-		good_pmbr = is_pmbr_valid(legacymbr, total_sectors);
+		good_pmbr = is_pmbr_valid(legacymbr);
 		kfree(legacymbr);
 
 		if (!good_pmbr)
-- 
1.8.1.4

Re: [PATCH 2/2 for 3.12-rc6] Revert "partitions/efi: account for pmbr size in lba"

[Andrew Morton]

On Mon, 14 Oct 2013 14:41:49 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:

> This reverts commit 27a7c642174eaec627f6a3a254035bf8abd02c5e.
> 
> This patch breaks existing systems, for example, Tizen IVI images do not boot
> the way they have always been booted because of this patch.

Does Doug's patch fix this?


From: Doug Anderson <dianders@chromium.org>
Subject: block/partitions/efi.c: treat size mismatch as a warning, not an error

In 27a7c64 ("partitions/efi: account for pmbr size in lba") we started
treating bad sizes in lba field of the partition that has the 0xEE (GPT
protective) as errors.  However, we may run into these "bad sizes" in the
real world if someone uses dd to copy an image from a smaller disk to a
bigger disk.  Since this case used to work (even without using force_gpt),
keep it working and treat the size mismatch as a warning instead of an
error.

Reported-by: Josh Triplett <josh@joshtriplett.org>
Reported-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Acked-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 block/partitions/efi.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff -puN block/partitions/efi.c~block-partitions-efic-treat-size-mismatch-as-a-warning-not-an-error block/partitions/efi.c
--- a/block/partitions/efi.c~block-partitions-efic-treat-size-mismatch-as-a-warning-not-an-error
+++ a/block/partitions/efi.c
@@ -222,11 +222,16 @@ check_hybrid:
 	 * the disk size.
 	 *
 	 * Hybrid MBRs do not necessarily comply with this.
+	 *
+	 * Consider a bad value here to be a warning to support dd'ing
+	 * an image from a smaller disk to a larger disk.
 	 */
 	if (ret == GPT_MBR_PROTECTIVE) {
 		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
 		if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
-			ret = 0;
+			pr_debug("GPT: mbr size in lba (%u) different than whole disk (%u).\n",
+				 sz, min_t(uint32_t,
+					   total_sectors - 1, 0xFFFFFFFF));
 	}
 done:
 	return ret;
_

Re: [PATCH 2/2 for 3.12-rc6] Revert "partitions/efi: account for pmbr size in lba"

[Artem Bityutskiy]

On Mon, 2013-10-14 at 11:26 -0700, Andrew Morton wrote:
> On Mon, 14 Oct 2013 14:41:49 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 
> > This reverts commit 27a7c642174eaec627f6a3a254035bf8abd02c5e.
> > 
> > This patch breaks existing systems, for example, Tizen IVI images do not boot
> > the way they have always been booted because of this patch.
> 
> Does Doug's patch fix this?

Yes, thanks.

-- 
Best Regards,
Artem Bityutskiy