[PATCH 1/2] part: Check all partitions in part_get_info_by_name()

[PATCH 1/2] part: Check all partitions in part_get_info_by_name()

[Sam Protsenko]

In part_get_info_by_name() the inability to get some partition info
shouldn't be a reason for dropping out of the loop. That might happen
e.g. if the partition is hidden or unused. An example of such case are
Samsung devices, where they use the "unused" GUID type
(00000000-0000-0000-0000-000000000000) to indicate that the partition
should be hidden from the OS. Such partitions might not be seen in
"part list" output, which creates "gaps" in numbering in between of the
visible partitions:

    Part    Start LBA       End LBA         Name
      1     0x00000400      0x0000a3ff      "efs"
      5     0x00026420      0x00026c1f      "dtbo"
     12     0x0003f390      0x0074738f      "super"

In that case, the loop in part_get_info_by_name() would break after
partition #1, so any attempt to obtain "dtbo" or "super" partition will
fail. Fix that by continuing to iterate over the remaining partitions to
make sure none of the visible ones is missed. That makes "part" command
(e.g. "part start", "part size") able to work with such tables.

Fixes: 87b8530fe244 ("disk: part: implement generic function part_get_info_by_name()")
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 disk/part.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/disk/part.c b/disk/part.c
index 36b88205eca7..08c9331e059c 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -717,8 +717,11 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
 	for (i = 1; i < part_drv->max_entries; i++) {
 		ret = part_drv->get_info(desc, i, info);
 		if (ret != 0) {
-			/* no more entries in table */
-			break;
+			/*
+			 * Partition with this index can't be obtained, but
+			 * further partitions might be, so keep checking.
+			 */
+			continue;
 		}
 		if (strcmp(name, (const char *)info->name) == 0) {
 			/* matched */
-- 
2.39.2

[PATCH 2/2] part: efi: Treat unused partition type GUID as a valid case

[Sam Protsenko]

Some platforms use the "unused" (all-zero) GUID as a partition type GUID
to make some partitions hidden from the OS. For example, Samsung phones
and other devices often have GPT partition tables like that, created by
their "gpt_builder" tool [1]. All partitions with FILESYS=0 value
(second column in [2] file) will be created in a way that:
  1. Partition type GUID will be all-zero ("unused")
  2. Attributes[48:49] bits will be set to 0 (whereas non-zero values
     mean the partition is visible to the OS: 1=raw, 2=ext4, 3=f2fs)

Although it's true that Linux kernel verifies the partition type GUID to
be non-zero (in block/partitions/efi.c, is_pte_valid() function), where
its U-Boot counterpart code was borrowed from originally, in case of
U-Boot we want to handle partitions with "unused" GUIDs the same way as
any other valid partitions, to allow the user to interact with those
(e.g. list partitions using "part list", be able to flash those via
fastboot, etc).

[1] https://gitlab.com/Linaro/96boards/e850-96/tools/gpt/
[2] https://gitlab.com/Linaro/96boards/e850-96/tools/gpt/-/blob/master/gpt_layout

Fixes: 07f3d789b9be ("Add support for CONFIG_EFI_PARTITION (GUID Partition Table)")
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 disk/part_efi.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 4ce9243ef25c..6b138abae0a6 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -1166,28 +1166,17 @@ static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
  */
 static int is_pte_valid(gpt_entry * pte)
 {
-	efi_guid_t unused_guid;
+	/*
+	 * NOTE: Do not check unused (zero) GUIDs here, it's considered a valid
+	 * case in U-Boot.
+	 */
 
 	if (!pte) {
 		log_debug("Invalid Argument(s)\n");
 		return 0;
 	}
 
-	/* Only one validation for now:
-	 * The GUID Partition Type != Unused Entry (ALL-ZERO)
-	 */
-	memset(unused_guid.b, 0, sizeof(unused_guid.b));
-
-	if (memcmp(pte->partition_type_guid.b, unused_guid.b,
-		sizeof(unused_guid.b)) == 0) {
-
-		log_debug("Found an unused PTE GUID at 0x%08X\n",
-			  (unsigned int)(uintptr_t)pte);
-
-		return 0;
-	} else {
-		return 1;
-	}
+	return 1;
 }
 
 /*
-- 
2.39.2

Re: [PATCH 1/2] part: Check all partitions in part_get_info_by_name()

[Heinrich Schuchardt]

On 3/28/24 23:29, Sam Protsenko wrote:
> In part_get_info_by_name() the inability to get some partition info
> shouldn't be a reason for dropping out of the loop. That might happen
> e.g. if the partition is hidden or unused. An example of such case are
> Samsung devices, where they use the "unused" GUID type
> (00000000-0000-0000-0000-000000000000) to indicate that the partition
> should be hidden from the OS. Such partitions might not be seen in
> "part list" output, which creates "gaps" in numbering in between of the
> visible partitions:
>
>      Part    Start LBA       End LBA         Name
>        1     0x00000400      0x0000a3ff      "efs"
>        5     0x00026420      0x00026c1f      "dtbo"
>       12     0x0003f390      0x0074738f      "super"
>
> In that case, the loop in part_get_info_by_name() would break after
> partition #1, so any attempt to obtain "dtbo" or "super" partition will
> fail. Fix that by continuing to iterate over the remaining partitions to
> make sure none of the visible ones is missed. That makes "part" command
> (e.g. "part start", "part size") able to work with such tables.
>
> Fixes: 87b8530fe244 ("disk: part: implement generic function part_get_info_by_name()")
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   disk/part.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 36b88205eca7..08c9331e059c 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -717,8 +717,11 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
>   	for (i = 1; i < part_drv->max_entries; i++) {
>   		ret = part_drv->get_info(desc, i, info);
>   		if (ret != 0) {
> -			/* no more entries in table */
> -			break;
> +			/*
> +			 * Partition with this index can't be obtained, but
> +			 * further partitions might be, so keep checking.
> +			 */
> +			continue;
>   		}
>   		if (strcmp(name, (const char *)info->name) == 0) {
>   			/* matched */

Re: [PATCH 2/2] part: efi: Treat unused partition type GUID as a valid case

[Heinrich Schuchardt]

On 3/28/24 23:29, Sam Protsenko wrote:
> Some platforms use the "unused" (all-zero) GUID as a partition type GUID
> to make some partitions hidden from the OS. For example, Samsung phones
> and other devices often have GPT partition tables like that, created by
> their "gpt_builder" tool [1]. All partitions with FILESYS=0 value
> (second column in [2] file) will be created in a way that:
>    1. Partition type GUID will be all-zero ("unused")
>    2. Attributes[48:49] bits will be set to 0 (whereas non-zero values
>       mean the partition is visible to the OS: 1=raw, 2=ext4, 3=f2fs)

The UEFI specification is defining what a GPT partition table has to
look like.

According the specification partition type GUID
00000000-0000-0000-0000-000000000000 marks an "unused entry" in the
partition table.

An unused partition table entry cannot define a partition. It is one of
the entries, that you skip over when enumerating via your patch 1/2.

With this patch 128 partition table entries are printed for an image
having a single partition.

=> part list host 0

Partition Map for HOST device 0  --   Partition Type: EFI

Part    Start LBA       End LBA         Name
         Attributes
         Type GUID
         Partition GUID
   1     0x00000800      0x0001f7ff      "EFI system partition"
         attrs:  0x0000000000000005
         type:   c12a7328-f81f-11d2-ba4b-00a0c93ec93b
                 (system)
         guid:   ee474198-4601-4d5c-81f0-54acf904dd40
   2     0x00000000      0x00000000      ""
         attrs:  0x0000000000000000
         type:   00000000-0000-0000-0000-000000000000
                 (00000000-0000-0000-0000-000000000000)
         guid:   00000000-0000-0000-0000-000000000000
...

128     0x00000000      0x00000000      ""
         attrs:  0x0000000000000000
         type:   00000000-0000-0000-0000-000000000000
                 (00000000-0000-0000-0000-000000000000)
         guid:   00000000-0000-0000-0000-000000000000

This is definitively wrong.

>
> Although it's true that Linux kernel verifies the partition type GUID to
> be non-zero (in block/partitions/efi.c, is_pte_valid() function), where
> its U-Boot counterpart code was borrowed from originally, in case of
> U-Boot we want to handle partitions with "unused" GUIDs the same way as
> any other valid partitions, to allow the user to interact with those
> (e.g. list partitions using "part list", be able to flash those via
> fastboot, etc).

You cannot interact with a non-existing partition.

You may create a new partition in any empty slot that Samsung's tool has
left in the partition table and then write to it. No patch is needed for
this.

Best regards

Heinrich

>
> [1] https://gitlab.com/Linaro/96boards/e850-96/tools/gpt/
> [2] https://gitlab.com/Linaro/96boards/e850-96/tools/gpt/-/blob/master/gpt_layout
>
> Fixes: 07f3d789b9be ("Add support for CONFIG_EFI_PARTITION (GUID Partition Table)")
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   disk/part_efi.c | 21 +++++----------------
>   1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 4ce9243ef25c..6b138abae0a6 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -1166,28 +1166,17 @@ static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
>    */
>   static int is_pte_valid(gpt_entry * pte)
>   {
> -	efi_guid_t unused_guid;
> +	/*
> +	 * NOTE: Do not check unused (zero) GUIDs here, it's considered a valid
> +	 * case in U-Boot.
> +	 */
>
>   	if (!pte) {
>   		log_debug("Invalid Argument(s)\n");
>   		return 0;
>   	}
>
> -	/* Only one validation for now:
> -	 * The GUID Partition Type != Unused Entry (ALL-ZERO)
> -	 */
> -	memset(unused_guid.b, 0, sizeof(unused_guid.b));
> -
> -	if (memcmp(pte->partition_type_guid.b, unused_guid.b,
> -		sizeof(unused_guid.b)) == 0) {
> -
> -		log_debug("Found an unused PTE GUID at 0x%08X\n",
> -			  (unsigned int)(uintptr_t)pte);
> -
> -		return 0;
> -	} else {
> -		return 1;
> -	}
> +	return 1;
>   }
>
>   /*

Re: [PATCH 2/2] part: efi: Treat unused partition type GUID as a valid case

[Sam Protsenko]

On Thu, Mar 28, 2024 at 8:07 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/28/24 23:29, Sam Protsenko wrote:
> > Some platforms use the "unused" (all-zero) GUID as a partition type GUID
> > to make some partitions hidden from the OS. For example, Samsung phones
> > and other devices often have GPT partition tables like that, created by
> > their "gpt_builder" tool [1]. All partitions with FILESYS=0 value
> > (second column in [2] file) will be created in a way that:
> >    1. Partition type GUID will be all-zero ("unused")
> >    2. Attributes[48:49] bits will be set to 0 (whereas non-zero values
> >       mean the partition is visible to the OS: 1=raw, 2=ext4, 3=f2fs)
>
> The UEFI specification is defining what a GPT partition table has to
> look like.
>
> According the specification partition type GUID
> 00000000-0000-0000-0000-000000000000 marks an "unused entry" in the
> partition table.
>
> An unused partition table entry cannot define a partition. It is one of
> the entries, that you skip over when enumerating via your patch 1/2.
>
> With this patch 128 partition table entries are printed for an image
> having a single partition.
>
> => part list host 0
>
> Partition Map for HOST device 0  --   Partition Type: EFI
>
> Part    Start LBA       End LBA         Name
>          Attributes
>          Type GUID
>          Partition GUID
>    1     0x00000800      0x0001f7ff      "EFI system partition"
>          attrs:  0x0000000000000005
>          type:   c12a7328-f81f-11d2-ba4b-00a0c93ec93b
>                  (system)
>          guid:   ee474198-4601-4d5c-81f0-54acf904dd40
>    2     0x00000000      0x00000000      ""
>          attrs:  0x0000000000000000
>          type:   00000000-0000-0000-0000-000000000000
>                  (00000000-0000-0000-0000-000000000000)
>          guid:   00000000-0000-0000-0000-000000000000
> ...
>
> 128     0x00000000      0x00000000      ""
>          attrs:  0x0000000000000000
>          type:   00000000-0000-0000-0000-000000000000
>                  (00000000-0000-0000-0000-000000000000)
>          guid:   00000000-0000-0000-0000-000000000000
>
> This is definitively wrong.
>
> >
> > Although it's true that Linux kernel verifies the partition type GUID to
> > be non-zero (in block/partitions/efi.c, is_pte_valid() function), where
> > its U-Boot counterpart code was borrowed from originally, in case of
> > U-Boot we want to handle partitions with "unused" GUIDs the same way as
> > any other valid partitions, to allow the user to interact with those
> > (e.g. list partitions using "part list", be able to flash those via
> > fastboot, etc).
>
> You cannot interact with a non-existing partition.
>
> You may create a new partition in any empty slot that Samsung's tool has
> left in the partition table and then write to it. No patch is needed for
> this.
>

Thanks for reviewing! In case of Samsung's way of creating GPT tables,
those are actually real partitions (they just "hide" those by means of
setting their type GUIDs to 0), and it's possible to flash those and
interact with them in other ways. But I can see how it's not a
standard way of doing things. And because (as you pointed out) this
patch leads to undesirable effects on other platforms, I also think it
should be NAKed. That of course means it won't be possible to access
all partitions on downstream Samsung devices, but with upstream U-Boot
people will probably want to create an appropriate GPT table anyways.

> Best regards
>
> Heinrich
>

[snip]

Re: [PATCH 1/2] part: Check all partitions in part_get_info_by_name()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

On Thu, Mar 28, 2024 at 05:29:50PM -0500, Sam Protsenko wrote:

> In part_get_info_by_name() the inability to get some partition info
> shouldn't be a reason for dropping out of the loop. That might happen
> e.g. if the partition is hidden or unused. An example of such case are
> Samsung devices, where they use the "unused" GUID type
> (00000000-0000-0000-0000-000000000000) to indicate that the partition
> should be hidden from the OS. Such partitions might not be seen in
> "part list" output, which creates "gaps" in numbering in between of the
> visible partitions:
> 
>     Part    Start LBA       End LBA         Name
>       1     0x00000400      0x0000a3ff      "efs"
>       5     0x00026420      0x00026c1f      "dtbo"
>      12     0x0003f390      0x0074738f      "super"
> 
> In that case, the loop in part_get_info_by_name() would break after
> partition #1, so any attempt to obtain "dtbo" or "super" partition will
> fail. Fix that by continuing to iterate over the remaining partitions to
> make sure none of the visible ones is missed. That makes "part" command
> (e.g. "part start", "part size") able to work with such tables.
> 
> Fixes: 87b8530fe244 ("disk: part: implement generic function part_get_info_by_name()")
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]