[U-Boot] [PATCH v2 0/4] Misc EFI/GPT/UUID fixes

[U-Boot] [PATCH v2 0/4] Misc EFI/GPT/UUID fixes

[Eugeniu Rosca]

This is a collection of fixes addressing issues on R-Car H3ULCB-KF.
All have been tested on H3ULCB-KF and boot-tested on sandbox.

Changes in v2:
 - Reworded the description and summary line of
   ("lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y")
   to emphasize it is a fix rather than an improvement.
 - No code changes.

Eugeniu Rosca (4):
  disk: efi: Fix memory leak on 'gpt guid'
  disk: efi: Fix memory leak on 'gpt verify'
  cmd: gpt: fix and tidy up help message
  lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y

 cmd/gpt.c       | 12 ++++++------
 disk/part_efi.c |  6 ++++++
 lib/uuid.c      |  2 ++
 3 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v2 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Eugeniu Rosca]

Below is what happens on R-Car H3ULCB-KF using clean U-Boot
v2019.04-00810-g6aebc0d11a10 and r8a7795_ulcb_defconfig:

 => ### interrupt autoboot
 => gpt guid mmc 1
 21200400-0804-0146-9dcc-a8c51255994f
 success!
 => ### keep calling 'gpt guid mmc 1'
 => ### on 59th call, we are out of memory:
 => gpt guid mmc 1
 alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT Entries
 GPT: Failed to allocate memory for PTE
 get_disk_guid: *** ERROR: Invalid GPT ***
 alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT Entries
 GPT: Failed to allocate memory for PTE
 get_disk_guid: *** ERROR: Invalid Backup GPT ***
 error!

After some inspection, it looks like get_disk_guid(), added via v2017.09
commit 73d6d18b7147c9 ("GPT: add accessor function for disk GUID"),
unlike other callers of is_gpt_valid(), doesn't free the memory pointed
out by 'gpt_entry *gpt_pte'. The latter is allocated by is_gpt_valid()
via alloc_read_gpt_entries().

With the fix applied, the reproduction scenario has been run hundreds
of times ('while true; do gpt guid mmc 1; done') w/o running into OOM.

Fixes: 73d6d18b7147c9 ("GPT: add accessor function for disk GUID")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
--
v2:
 - Added Reviewed-by: Heinrich Schuchardt
v1:
 - https://patchwork.ozlabs.org/patch/1092942/
---
 disk/part_efi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 239455b8161e..812d14cdd871 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -209,6 +209,8 @@ int get_disk_guid(struct blk_desc * dev_desc, char *guid)
 	guid_bin = gpt_head->disk_guid.b;
 	uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
 
+	/* Remember to free pte */
+	free(gpt_pte);
 	return 0;
 }
 
-- 
2.21.0

[U-Boot] [PATCH v2 2/4] disk: efi: Fix memory leak on 'gpt verify'

[Eugeniu Rosca]

Below is what happens on R-Car H3ULCB-KF using clean U-Boot
v2019.04-00810-g6aebc0d11a10 and r8a7795_ulcb_defconfig:

 => ### interrupt autoboot
 => gpt verify mmc 1
 No partition list provided - only basic check
 Verify GPT: success!
 => ### keep calling 'gpt verify mmc 1'
 => ### on 58th call, we are out of memory:
 => gpt verify mmc 1
 alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT Entries
 GPT: Failed to allocate memory for PTE
 gpt_verify_headers: *** ERROR: Invalid Backup GPT ***
 Verify GPT: error!

This is caused by calling is_gpt_valid() twice (hence allocating pte
also twice via alloc_read_gpt_entries()) while freeing pte only _once_
in the caller of gpt_verify_headers(). Fix that by freeing the pte
allocated and populated for primary GPT _before_ allocating and
populating the pte for backup GPT. The latter will be freed by the
caller of gpt_verify_headers().

With the fix applied, the reproduction scenario [1-2] has been run
hundreds of times in a loop w/o running into OOM.

[1] gpt verify mmc 1
[2] gpt verify mmc 1 $partitions

Fixes: cef68bf9042dda ("gpt: part: Definition and declaration of GPT verification functions")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
--
v2:
 - Added Reviewed-by: Heinrich Schuchardt
v1:
 - https://patchwork.ozlabs.org/patch/1092943/
---
 disk/part_efi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 812d14cdd871..c0fa753339c8 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -698,6 +698,10 @@ int gpt_verify_headers(struct blk_desc *dev_desc, gpt_header *gpt_head,
 		       __func__);
 		return -1;
 	}
+
+	/* Free pte before allocating again */
+	free(*gpt_pte);
+
 	if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
 			 gpt_head, gpt_pte) != 1) {
 		printf("%s: *** ERROR: Invalid Backup GPT ***\n",
-- 
2.21.0

[U-Boot] [PATCH v2 3/4] cmd: gpt: fix and tidy up help message

[Eugeniu Rosca]

Apply the following changes:
 - Guard the 'gpt read' command by 'ifdef CONFIG_CMD_GPT_RENAME',
   since 'gpt read' is not available on CMD_GPT_RENAME=n
 - Prefix the {read,swap,rename} commands with one space for consistency
 - Prefix the 'guid' commands with 'gpt' for consistency

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
--
v2:
 - Added Reviewed-by: Heinrich Schuchardt
v1:
 - https://patchwork.ozlabs.org/patch/1092944/
---
 cmd/gpt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index 638870352f40..33cda513969f 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -876,21 +876,21 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
 	" Example usage:\n"
 	" gpt write mmc 0 $partitions\n"
 	" gpt verify mmc 0 $partitions\n"
-	" read <interface> <dev>\n"
-	"    - read GPT into a data structure for manipulation\n"
-	" guid <interface> <dev>\n"
+	" gpt guid <interface> <dev>\n"
 	"    - print disk GUID\n"
-	" guid <interface> <dev> <varname>\n"
+	" gpt guid <interface> <dev> <varname>\n"
 	"    - set environment variable to disk GUID\n"
 	" Example usage:\n"
 	" gpt guid mmc 0\n"
 	" gpt guid mmc 0 varname\n"
 #ifdef CONFIG_CMD_GPT_RENAME
 	"gpt partition renaming commands:\n"
-	"gpt swap <interface> <dev> <name1> <name2>\n"
+	" gpt read <interface> <dev>\n"
+	"    - read GPT into a data structure for manipulation\n"
+	" gpt swap <interface> <dev> <name1> <name2>\n"
 	"    - change all partitions named name1 to name2\n"
 	"      and vice-versa\n"
-	"gpt rename <interface> <dev> <part> <name>\n"
+	" gpt rename <interface> <dev> <part> <name>\n"
 	"    - rename the specified partition\n"
 	" Example usage:\n"
 	" gpt swap mmc 0 foo bar\n"
-- 
2.21.0

[U-Boot] [PATCH v2 4/4] lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y

[Eugeniu Rosca]

The random uuid values (enabled via CONFIG_RANDOM_UUID=y) on our
platform are always the same. Below is consistent on each cold boot:

 => ### interrupt autoboot
 => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
 ...
 uuid_gpt_misc=d117f98e-6f2c-d04b-a5b2-331a19f91cb2
 => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
 ...
 uuid_gpt_misc=ad5ec4b6-2d9f-8544-9417-fe3bd1c9b1b3
 => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
 ...
 uuid_gpt_misc=cceb0b18-39cb-d547-9db7-03b405fa77d4
 => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
 ...
 uuid_gpt_misc=d4981a2b-0478-544e-9607-7fd3c651068d
 => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
 ...
 uuid_gpt_misc=6d6c9a36-e919-264d-a9ee-bd00379686c7

While the uuids do change on every 'gpt write' command, the values
appear to be taken from the same pool, in the same order.

Assuming U-Boot with RANDOM_UUID=y is deployed on a large number of
devices, all those devices would essentially expose the same UUID,
breaking the assumption of system/RFS/application designers who rely
on UUID as being globally unique (e.g. a database using UUID as key
would alias/mix up entries/records due to duplicated UUID).

The root cause seems to be simply _not_ seeding PRNG before generating
a random value. It turns out this belongs to an established class of
PRNG-specific problems, commonly known as "unseeded randomness", for
which I am able to find below bugs/CVE/CWE:
 - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-0285
   ("CVE-2015-0285 openssl: handshake with unseeded PRNG")
 - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-9019
   ("CVE-2015-9019 libxslt: math.random() in xslt uses unseeded
   randomness")
 - https://cwe.mitre.org/data/definitions/336.html
   ("CWE-336: Same Seed in Pseudo-Random Number Generator (PRNG)")

The first revision [1] of this patch updated the seed based on the
output of get_timer(), similar to [4].

There are two problems with this approach:
 - get_timer() has a poor _ms_ resolution
 - when gen_rand_uuid() is called in a loop, get_timer() returns the
   same result, leading to the same seed being passed to srand(),
   leading to the same uuid being generated for several partitions
   with different names

The above drawbacks have been addressed in the second version [2].
In its third revision (current), the patch reworded the description
and summary line to emphasize it is a *fix* rather than an improvement.

Testing [3] consisted of running 'gpt write mmc 1 $partitions' in a
loop on R-Car3 for several minutes, collecting 8844 randomly generated
UUIDS. Two consecutive cold boots are concatenated in the log.
As a result, all uuid values are unique (scripted check).

Thanks to Roman, who reported the issue and provided support in fixing.

[1] https://patchwork.ozlabs.org/patch/1091802/
[2] https://patchwork.ozlabs.org/patch/1092945/
[3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
[4] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")

Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
--
v3:
 - Reworked the patch summary line and description to emphasize this is
   a fix rather than an improvement by precisely pointing out the root
   cause and mentioning related CVE/CWE.
v2:
 - https://patchwork.ozlabs.org/patch/1092945/
 - Replaced get_timer(0) with get_ticks() and added rand() to seed value
 - Performed extensive testing on R-Car3 (ARMv8). See:
   https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
v1:
 - https://patchwork.ozlabs.org/patch/1092944/
---
 lib/uuid.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/uuid.c b/lib/uuid.c
index fa20ee39fc32..2d4d6ef7e461 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -238,6 +238,8 @@ void gen_rand_uuid(unsigned char *uuid_bin)
 	unsigned int *ptr = (unsigned int *)&uuid;
 	int i;
 
+	srand(get_ticks() + rand());
+
 	/* Set all fields randomly */
 	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
 		*(ptr + i) = cpu_to_be32(rand());
-- 
2.21.0

[U-Boot] [PATCH v2 4/4] lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y

[Heinrich Schuchardt]

On 5/2/19 2:27 PM, Eugeniu Rosca wrote:
> The random uuid values (enabled via CONFIG_RANDOM_UUID=y) on our
> platform are always the same. Below is consistent on each cold boot:
>
>   => ### interrupt autoboot
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=d117f98e-6f2c-d04b-a5b2-331a19f91cb2
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=ad5ec4b6-2d9f-8544-9417-fe3bd1c9b1b3
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=cceb0b18-39cb-d547-9db7-03b405fa77d4
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=d4981a2b-0478-544e-9607-7fd3c651068d
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=6d6c9a36-e919-264d-a9ee-bd00379686c7
>
> While the uuids do change on every 'gpt write' command, the values
> appear to be taken from the same pool, in the same order.
>
> Assuming U-Boot with RANDOM_UUID=y is deployed on a large number of
> devices, all those devices would essentially expose the same UUID,
> breaking the assumption of system/RFS/application designers who rely
> on UUID as being globally unique (e.g. a database using UUID as key
> would alias/mix up entries/records due to duplicated UUID).
>
> The root cause seems to be simply _not_ seeding PRNG before generating
> a random value. It turns out this belongs to an established class of
> PRNG-specific problems, commonly known as "unseeded randomness", for
> which I am able to find below bugs/CVE/CWE:
>   - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-0285
>     ("CVE-2015-0285 openssl: handshake with unseeded PRNG")
>   - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-9019
>     ("CVE-2015-9019 libxslt: math.random() in xslt uses unseeded
>     randomness")
>   - https://cwe.mitre.org/data/definitions/336.html
>     ("CWE-336: Same Seed in Pseudo-Random Number Generator (PRNG)")
>
> The first revision [1] of this patch updated the seed based on the
> output of get_timer(), similar to [4].
>
> There are two problems with this approach:
>   - get_timer() has a poor _ms_ resolution
>   - when gen_rand_uuid() is called in a loop, get_timer() returns the
>     same result, leading to the same seed being passed to srand(),
>     leading to the same uuid being generated for several partitions
>     with different names
>
> The above drawbacks have been addressed in the second version [2].
> In its third revision (current), the patch reworded the description
> and summary line to emphasize it is a *fix* rather than an improvement.
>
> Testing [3] consisted of running 'gpt write mmc 1 $partitions' in a
> loop on R-Car3 for several minutes, collecting 8844 randomly generated
> UUIDS. Two consecutive cold boots are concatenated in the log.
> As a result, all uuid values are unique (scripted check).
>
> Thanks to Roman, who reported the issue and provided support in fixing.
>
> [1] https://patchwork.ozlabs.org/patch/1091802/
> [2] https://patchwork.ozlabs.org/patch/1092945/
> [3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> [4] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")
>
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> --
> v3:
>   - Reworked the patch summary line and description to emphasize this is
>     a fix rather than an improvement by precisely pointing out the root
>     cause and mentioning related CVE/CWE.
> v2:
>   - https://patchwork.ozlabs.org/patch/1092945/
>   - Replaced get_timer(0) with get_ticks() and added rand() to seed value
>   - Performed extensive testing on R-Car3 (ARMv8). See:
>     https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> v1:
>   - https://patchwork.ozlabs.org/patch/1092944/
> ---
>   lib/uuid.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/lib/uuid.c b/lib/uuid.c
> index fa20ee39fc32..2d4d6ef7e461 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -238,6 +238,8 @@ void gen_rand_uuid(unsigned char *uuid_bin)
>   	unsigned int *ptr = (unsigned int *)&uuid;
>   	int i;
>
> +	srand(get_ticks() + rand());
> +
>   	/* Set all fields randomly */
>   	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
>   		*(ptr + i) = cpu_to_be32(rand());
>

[U-Boot] [PATCH v2 3/4] cmd: gpt: fix and tidy up help message

[Lukasz Majewski]

On Thu, 2 May 2019 14:27:05 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> Apply the following changes:
>  - Guard the 'gpt read' command by 'ifdef CONFIG_CMD_GPT_RENAME',
>    since 'gpt read' is not available on CMD_GPT_RENAME=n
>  - Prefix the {read,swap,rename} commands with one space for
> consistency
>  - Prefix the 'guid' commands with 'gpt' for consistency
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> --
> v2:
>  - Added Reviewed-by: Heinrich Schuchardt
> v1:
>  - https://patchwork.ozlabs.org/patch/1092944/
> ---
>  cmd/gpt.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 638870352f40..33cda513969f 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -876,21 +876,21 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>  	" Example usage:\n"
>  	" gpt write mmc 0 $partitions\n"
>  	" gpt verify mmc 0 $partitions\n"
> -	" read <interface> <dev>\n"
> -	"    - read GPT into a data structure for manipulation\n"
> -	" guid <interface> <dev>\n"
> +	" gpt guid <interface> <dev>\n"
>  	"    - print disk GUID\n"
> -	" guid <interface> <dev> <varname>\n"
> +	" gpt guid <interface> <dev> <varname>\n"
>  	"    - set environment variable to disk GUID\n"
>  	" Example usage:\n"
>  	" gpt guid mmc 0\n"
>  	" gpt guid mmc 0 varname\n"
>  #ifdef CONFIG_CMD_GPT_RENAME
>  	"gpt partition renaming commands:\n"
> -	"gpt swap <interface> <dev> <name1> <name2>\n"
> +	" gpt read <interface> <dev>\n"
> +	"    - read GPT into a data structure for manipulation\n"
> +	" gpt swap <interface> <dev> <name1> <name2>\n"
>  	"    - change all partitions named name1 to name2\n"
>  	"      and vice-versa\n"
> -	"gpt rename <interface> <dev> <part> <name>\n"
> +	" gpt rename <interface> <dev> <part> <name>\n"
>  	"    - rename the specified partition\n"
>  	" Example usage:\n"
>  	" gpt swap mmc 0 foo bar\n"

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/d4531ba8/attachment.sig>

[U-Boot] [PATCH v2 2/4] disk: efi: Fix memory leak on 'gpt verify'

[Lukasz Majewski]

On Thu, 2 May 2019 14:27:04 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> Below is what happens on R-Car H3ULCB-KF using clean U-Boot
> v2019.04-00810-g6aebc0d11a10 and r8a7795_ulcb_defconfig:
> 
>  => ### interrupt autoboot
>  => gpt verify mmc 1  
>  No partition list provided - only basic check
>  Verify GPT: success!
>  => ### keep calling 'gpt verify mmc 1'
>  => ### on 58th call, we are out of memory:
>  => gpt verify mmc 1  
>  alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT
> Entries GPT: Failed to allocate memory for PTE
>  gpt_verify_headers: *** ERROR: Invalid Backup GPT ***
>  Verify GPT: error!
> 
> This is caused by calling is_gpt_valid() twice (hence allocating pte
> also twice via alloc_read_gpt_entries()) while freeing pte only _once_
> in the caller of gpt_verify_headers(). Fix that by freeing the pte
> allocated and populated for primary GPT _before_ allocating and
> populating the pte for backup GPT. The latter will be freed by the
> caller of gpt_verify_headers().
> 
> With the fix applied, the reproduction scenario [1-2] has been run
> hundreds of times in a loop w/o running into OOM.
> 
> [1] gpt verify mmc 1
> [2] gpt verify mmc 1 $partitions
> 
> Fixes: cef68bf9042dda ("gpt: part: Definition and declaration of GPT
> verification functions") Signed-off-by: Eugeniu Rosca
> <erosca@de.adit-jv.com> Reviewed-by: Heinrich Schuchardt
> <xypron.glpk@gmx.de> --
> v2:
>  - Added Reviewed-by: Heinrich Schuchardt
> v1:
>  - https://patchwork.ozlabs.org/patch/1092943/
> ---
>  disk/part_efi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 812d14cdd871..c0fa753339c8 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -698,6 +698,10 @@ int gpt_verify_headers(struct blk_desc
> *dev_desc, gpt_header *gpt_head, __func__);
>  		return -1;
>  	}
> +
> +	/* Free pte before allocating again */
> +	free(*gpt_pte);
> +
>  	if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
>  			 gpt_head, gpt_pte) != 1) {
>  		printf("%s: *** ERROR: Invalid Backup GPT ***\n",

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/61711568/attachment.sig>

[U-Boot] [PATCH v2 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Lukasz Majewski]

On Thu, 2 May 2019 14:27:03 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> Below is what happens on R-Car H3ULCB-KF using clean U-Boot
> v2019.04-00810-g6aebc0d11a10 and r8a7795_ulcb_defconfig:
> 
>  => ### interrupt autoboot
>  => gpt guid mmc 1  
>  21200400-0804-0146-9dcc-a8c51255994f
>  success!
>  => ### keep calling 'gpt guid mmc 1'
>  => ### on 59th call, we are out of memory:
>  => gpt guid mmc 1  
>  alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT
> Entries GPT: Failed to allocate memory for PTE
>  get_disk_guid: *** ERROR: Invalid GPT ***
>  alloc_read_gpt_entries: ERROR: Can't allocate 0X4000 bytes for GPT
> Entries GPT: Failed to allocate memory for PTE
>  get_disk_guid: *** ERROR: Invalid Backup GPT ***
>  error!
> 
> After some inspection, it looks like get_disk_guid(), added via
> v2017.09 commit 73d6d18b7147c9 ("GPT: add accessor function for disk
> GUID"), unlike other callers of is_gpt_valid(), doesn't free the
> memory pointed out by 'gpt_entry *gpt_pte'. The latter is allocated
> by is_gpt_valid() via alloc_read_gpt_entries().
> 
> With the fix applied, the reproduction scenario has been run hundreds
> of times ('while true; do gpt guid mmc 1; done') w/o running into OOM.
> 
> Fixes: 73d6d18b7147c9 ("GPT: add accessor function for disk GUID")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> --
> v2:
>  - Added Reviewed-by: Heinrich Schuchardt
> v1:
>  - https://patchwork.ozlabs.org/patch/1092942/
> ---
>  disk/part_efi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 239455b8161e..812d14cdd871 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -209,6 +209,8 @@ int get_disk_guid(struct blk_desc * dev_desc,
> char *guid) guid_bin = gpt_head->disk_guid.b;
>  	uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
>  
> +	/* Remember to free pte */
> +	free(gpt_pte);
>  	return 0;
>  }
>  

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/efdd53a4/attachment.sig>

[U-Boot] [PATCH v2 4/4] lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y

[Lukasz Majewski]

On Thu, 2 May 2019 14:27:06 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> The random uuid values (enabled via CONFIG_RANDOM_UUID=y) on our
> platform are always the same. Below is consistent on each cold boot:
> 
>  => ### interrupt autoboot
>  => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc  
>  ...
>  uuid_gpt_misc=d117f98e-6f2c-d04b-a5b2-331a19f91cb2
>  => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc  
>  ...
>  uuid_gpt_misc=ad5ec4b6-2d9f-8544-9417-fe3bd1c9b1b3
>  => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc  
>  ...
>  uuid_gpt_misc=cceb0b18-39cb-d547-9db7-03b405fa77d4
>  => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc  
>  ...
>  uuid_gpt_misc=d4981a2b-0478-544e-9607-7fd3c651068d
>  => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc  
>  ...
>  uuid_gpt_misc=6d6c9a36-e919-264d-a9ee-bd00379686c7
> 
> While the uuids do change on every 'gpt write' command, the values
> appear to be taken from the same pool, in the same order.
> 
> Assuming U-Boot with RANDOM_UUID=y is deployed on a large number of
> devices, all those devices would essentially expose the same UUID,
> breaking the assumption of system/RFS/application designers who rely
> on UUID as being globally unique (e.g. a database using UUID as key
> would alias/mix up entries/records due to duplicated UUID).
> 
> The root cause seems to be simply _not_ seeding PRNG before generating
> a random value. It turns out this belongs to an established class of
> PRNG-specific problems, commonly known as "unseeded randomness", for
> which I am able to find below bugs/CVE/CWE:
>  - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-0285
>    ("CVE-2015-0285 openssl: handshake with unseeded PRNG")
>  - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-9019
>    ("CVE-2015-9019 libxslt: math.random() in xslt uses unseeded
>    randomness")
>  - https://cwe.mitre.org/data/definitions/336.html
>    ("CWE-336: Same Seed in Pseudo-Random Number Generator (PRNG)")
> 
> The first revision [1] of this patch updated the seed based on the
> output of get_timer(), similar to [4].
> 
> There are two problems with this approach:
>  - get_timer() has a poor _ms_ resolution
>  - when gen_rand_uuid() is called in a loop, get_timer() returns the
>    same result, leading to the same seed being passed to srand(),
>    leading to the same uuid being generated for several partitions
>    with different names
> 
> The above drawbacks have been addressed in the second version [2].
> In its third revision (current), the patch reworded the description
> and summary line to emphasize it is a *fix* rather than an
> improvement.
> 
> Testing [3] consisted of running 'gpt write mmc 1 $partitions' in a
> loop on R-Car3 for several minutes, collecting 8844 randomly generated
> UUIDS. Two consecutive cold boots are concatenated in the log.
> As a result, all uuid values are unique (scripted check).
> 
> Thanks to Roman, who reported the issue and provided support in
> fixing.
> 
> [1] https://patchwork.ozlabs.org/patch/1091802/
> [2] https://patchwork.ozlabs.org/patch/1092945/
> [3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> [4] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr()
> function")
> 
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> --
> v3:
>  - Reworked the patch summary line and description to emphasize this
> is a fix rather than an improvement by precisely pointing out the root
>    cause and mentioning related CVE/CWE.
> v2:
>  - https://patchwork.ozlabs.org/patch/1092945/
>  - Replaced get_timer(0) with get_ticks() and added rand() to seed
> value
>  - Performed extensive testing on R-Car3 (ARMv8). See:
>    https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> v1:
>  - https://patchwork.ozlabs.org/patch/1092944/
> ---
>  lib/uuid.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/uuid.c b/lib/uuid.c
> index fa20ee39fc32..2d4d6ef7e461 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -238,6 +238,8 @@ void gen_rand_uuid(unsigned char *uuid_bin)
>  	unsigned int *ptr = (unsigned int *)&uuid;
>  	int i;
>  
> +	srand(get_ticks() + rand());
> +
>  	/* Set all fields randomly */
>  	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
>  		*(ptr + i) = cpu_to_be32(rand());

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/5d233fd7/attachment.sig>

[U-Boot] [PATCH v2 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Eugeniu Rosca]

Hi Lukasz,

On Tue, May 07, 2019 at 09:03:15AM +0200, Lukasz Majewski wrote:
[..]
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de

Thanks for the recent reviews.
FYI, the patches from this series have already been accepted by Tom:

4ccf678f3773 ("lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y")
d02660960d78 ("cmd: gpt: fix and tidy up help message")
716f919d2da7 ("disk: efi: Fix memory leak on 'gpt verify'")
1cfe9694752e ("disk: efi: Fix memory leak on 'gpt guid'")

Thinking of some process optimization, wouldn't it be useful to see a
message from the delegated maintainer that the patches have been applied
to his U-Boot fork?

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Lukasz Majewski]

On Tue, 7 May 2019 09:10:28 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> Hi Lukasz,
> 
> On Tue, May 07, 2019 at 09:03:15AM +0200, Lukasz Majewski wrote:
> [..]
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de  
> 
> Thanks for the recent reviews.
> FYI, the patches from this series have already been accepted by Tom:
> 
> 4ccf678f3773 ("lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y")
> d02660960d78 ("cmd: gpt: fix and tidy up help message")
> 716f919d2da7 ("disk: efi: Fix memory leak on 'gpt verify'")
> 1cfe9694752e ("disk: efi: Fix memory leak on 'gpt guid'")
> 
> Thinking of some process optimization, wouldn't it be useful to see a
> message from the delegated maintainer that the patches have been
> applied to his U-Boot fork?

It would be helpful, but the person to be blamed is me :-) I shall
review the patches sooner.

Anyway, thanks for your work as they fix real issues.

> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/b2081fd1/attachment.sig>

[U-Boot] [PATCH v2 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Eugeniu Rosca]

On Tue, May 07, 2019 at 10:11:18AM +0200, Lukasz Majewski wrote:
> On Tue, 7 May 2019 09:10:28 +0200
> Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> 
> > Hi Lukasz,
> > 
> > On Tue, May 07, 2019 at 09:03:15AM +0200, Lukasz Majewski wrote:
> > [..]
> > > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > 
> > Thanks for the recent reviews.
> > FYI, the patches from this series have already been accepted by Tom:
> > 
> > 4ccf678f3773 ("lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y")
> > d02660960d78 ("cmd: gpt: fix and tidy up help message")
> > 716f919d2da7 ("disk: efi: Fix memory leak on 'gpt verify'")
> > 1cfe9694752e ("disk: efi: Fix memory leak on 'gpt guid'")
> > 
> > Thinking of some process optimization, wouldn't it be useful to see a
> > message from the delegated maintainer that the patches have been
> > applied to his U-Boot fork?
> 
> It would be helpful, but the person to be blamed is me :-) I shall
> review the patches sooner.

I'll make sure you will have another chance :-)

> 
> Anyway, thanks for your work as they fix real issues.
> 

-- 
Best Regards,
Eugeniu.