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

[U-Boot] [PATCH 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.

Below v1 patch is superseded by this series:
 - https://patchwork.ozlabs.org/patch/1091802/
 ("lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y")

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: Improve randomness of uuid values 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 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>
---
 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 1/4] disk: efi: Fix memory leak on 'gpt guid'

[Heinrich Schuchardt]

On 4/30/19 4:53 AM, Eugeniu Rosca 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>

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

[U-Boot] [PATCH 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>
---
 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 2/4] disk: efi: Fix memory leak on 'gpt verify'

[Heinrich Schuchardt]

On 4/30/19 4:53 AM, Eugeniu Rosca 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>

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

[U-Boot] [PATCH 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>
---
 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 3/4] cmd: gpt: fix and tidy up help message

[Heinrich Schuchardt]

On 4/30/19 4:53 AM, Eugeniu Rosca 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>

Non-related:
doc/README.commands describes the preferred way to implement sub-commands.

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

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values 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.

As a user, I expect a trully random uuid value in the above example.
Otherwise, system/RFS designers and OS people might assume they have
a reliable/consistent uuid passed by the bootloader, while the truth
is U-Boot simply lacks entropy to generate a random string.

In its first attempt [1] to improve the uuid randomness, this patch
updated the seed based on the output of get_timer(), similar to [2].

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

This second patch addresses both drawbacks.

My R-Car3 testing [3] consists of running 'gpt write mmc 1 $partitions'
in a loop 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] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")
[3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
 => while true; do \
    env default -a; \
    gpt write mmc 1 $partitions; \
    print; done

Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v2:
 - Replaced get_timer(0) with get_ticks() and added rand() to seed value
 - Performed extensive testing on R-Car3 (ARMv8)
v1:
 - https://patchwork.ozlabs.org/patch/1091802/
---
 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 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Heinrich Schuchardt]

On 4/30/19 4:53 AM, 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.
>
> As a user, I expect a trully random uuid value in the above example.
> Otherwise, system/RFS designers and OS people might assume they have
> a reliable/consistent uuid passed by the bootloader, while the truth
> is U-Boot simply lacks entropy to generate a random string.
>
> In its first attempt [1] to improve the uuid randomness, this patch
> updated the seed based on the output of get_timer(), similar to [2].
>
> 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
>
> This second patch addresses both drawbacks.
>
> My R-Car3 testing [3] consists of running 'gpt write mmc 1 $partitions'
> in a loop 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] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")
> [3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
>   => while true; do \
>      env default -a; \
>      gpt write mmc 1 $partitions; \
>      print; done
>
> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

This patch may ameliorate the situation for GUIDs a bit. But I dislike:

- This patch is a uuid only solution to introduce time ticks as source
   of entropy.
- With timer ticks you possibly introduce very little entropy.
- Our random number generator with only 32 state bits remains
   sub-standard.

This is the current situation:

net/bootp.c uses the MAC address to seed the random number generator and
uses random numbers for defining waits.

lib/uuid.c is using it for UUID generation.

In the UEFI sub-system I would like to implement the EFI_RNG_PROTOCOL.
Linux uses it for randomizing memory layout. iPXE needs it for secure
network connections. This requires a good random number generator with
sufficient entropy.

We already have implemented a single hardware random number generator in
drivers/crypto/ace_sha.c (CONFIG_EXYNOS_ACE_SHA).

Many other CPUs come with a hardware random number generator. In Linux's
drivers/char/hw_random/ I found, e.g.

- meson-rng.c (Amlogic)
- mtk-rng.c (MediaTek)
- st-rng.c (STMicroelectronics)
- imx-rng.c (Freescale)

I think we should have a u-class for hardware RNGs as one source of entropy.

I would like a random number generator with a high number of state bits
(> 127) that we initialize with hardware RNG bits and other sources of
entropy. A nice discussion of how Linux does it can be found in [1].

Best regards

Heinrich

[1]
https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf


> ---
> v2:
>   - Replaced get_timer(0) with get_ticks() and added rand() to seed value
>   - Performed extensive testing on R-Car3 (ARMv8)
> v1:
>   - https://patchwork.ozlabs.org/patch/1091802/
> ---
>   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 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Eugeniu Rosca]

Hi Heinrich,

Thank you for reviewing this series.

On Tue, Apr 30, 2019 at 09:07:09PM +0200, Heinrich Schuchardt wrote:
[..]
> This patch may ameliorate the situation for GUIDs a bit. But I dislike:

In general, we can find reasons to dislike anything, since there is room
for improvement in virtually anything.

> 
> - This patch is a uuid only solution to introduce time ticks as source
>   of entropy.

I would like to clarify once again what this patch is about. It _fixes_
(hence I will rewrite the summary line in the next patch revision) a
concrete real-life problem of providing repeatable (not predictable -
which implies some effort in my mind - but literally repeatable) uuid
values on enabling CONFIG_RANDOM_UUID.

It is my understanding that CONFIG_RANDOM_UUID (based on its name
and help message) does promise random uuids to the user. If so, then
U-Boot simply breaks this promise.

While doing additional research on PRNG, it looks to me that there is
an established class of PRNG-specific problems, commonly known as
"unseeded randomness" for which I am also able to find below 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 above tells me that using the same seed yields the same sequence
of random numbers. That's precisely the topic of this patch: simply
switching from unseeded PRNG to seeded PRNG.

And yes, this patch is deliberately limited to UUID naming function,
since it is lib/uuid's responsibility to seed the PRNG. The same is
true for other callers of rand() and rand_r(). All of them seed the
PRNG prior to getting a random value from it.

> - With timer ticks you possibly introduce very little entropy.

Theoretically, yes. Practically, the improvement to the current state
of affairs is huge and this has been testified by the test results
linked in the description. 

Again, this patch is not about improving the random pattern of the UUID
values (sorry for the misleading title). It is really about _enabling_
any kind of randomness in those values at all.

> - Our random number generator with only 32 state bits remains
>   sub-standard.

I believe this is orthogonal to my patch and can be improved
independently. In addition, whatever is the bit-width of U-Boot PRNG
(I can find shootouts between various 64/128/256-bit PRNG) its essence
will not change. It will remain a deterministic number generator,
whose randomness will still be dictated by the seed.

> 
> This is the current situation:
> 
> net/bootp.c uses the MAC address to seed the random number generator and
> uses random numbers for defining waits.
> 
> lib/uuid.c is using it for UUID generation.

I can see that rfc4122 suggests including MAC address in the UUID, but
it also warns that MAC address could be unavailable:

 -----------
   For systems with no IEEE address, a randomly or pseudo-randomly
   generated value may be used;
 -----------

The guess is right. Some SoCs like R-Car3 (in contrast to e.g. i.MX6)
don't provide any OTP memory/register containing their unique MAC
address. Needless to say some machines would never connect to IEEE
network. So, it looks to me that MAC address cannot be taken as
consistent source of entropy for UUID in U-Boot.

> 
> In the UEFI sub-system I would like to implement the EFI_RNG_PROTOCOL.
> Linux uses it for randomizing memory layout. iPXE needs it for secure
> network connections. This requires a good random number generator with
> sufficient entropy.
> 
> We already have implemented a single hardware random number generator in
> drivers/crypto/ace_sha.c (CONFIG_EXYNOS_ACE_SHA).
> 
> Many other CPUs come with a hardware random number generator. In Linux's
> drivers/char/hw_random/ I found, e.g.
> 
> - meson-rng.c (Amlogic)
> - mtk-rng.c (MediaTek)
> - st-rng.c (STMicroelectronics)
> - imx-rng.c (Freescale)

To the best of my knowledge, there is no HW RNG on R-Car3, so our only
option is currently U-Boot PRNG.

> 
> I think we should have a u-class for hardware RNGs as one source of entropy.
> 
> I would like a random number generator with a high number of state bits
> (> 127) that we initialize with hardware RNG bits and other sources of
> entropy. A nice discussion of how Linux does it can be found in [1].

All sound like future topics to me, orthogonal to this patch.
Let me know if I misunderstood anything. TIA!

> 
> Best regards
> 
> Heinrich
> 
> [1]
> https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf
> 

--
Best Regards,
Eugeniu.

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Tom Rini]

On Wed, May 01, 2019 at 09:08:31PM +0200, Eugeniu Rosca wrote:
> Hi Heinrich,
> 
> Thank you for reviewing this series.
> 
> On Tue, Apr 30, 2019 at 09:07:09PM +0200, Heinrich Schuchardt wrote:
> [..]
> > This patch may ameliorate the situation for GUIDs a bit. But I dislike:
> 
> In general, we can find reasons to dislike anything, since there is room
> for improvement in virtually anything.
> 
> > 
> > - This patch is a uuid only solution to introduce time ticks as source
> >   of entropy.
> 
> I would like to clarify once again what this patch is about. It _fixes_
> (hence I will rewrite the summary line in the next patch revision) a
> concrete real-life problem of providing repeatable (not predictable -
> which implies some effort in my mind - but literally repeatable) uuid
> values on enabling CONFIG_RANDOM_UUID.
> 
> It is my understanding that CONFIG_RANDOM_UUID (based on its name
> and help message) does promise random uuids to the user. If so, then
> U-Boot simply breaks this promise.
> 
> While doing additional research on PRNG, it looks to me that there is
> an established class of PRNG-specific problems, commonly known as
> "unseeded randomness" for which I am also able to find below 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 above tells me that using the same seed yields the same sequence
> of random numbers. That's precisely the topic of this patch: simply
> switching from unseeded PRNG to seeded PRNG.
> 
> And yes, this patch is deliberately limited to UUID naming function,
> since it is lib/uuid's responsibility to seed the PRNG. The same is
> true for other callers of rand() and rand_r(). All of them seed the
> PRNG prior to getting a random value from it.
> 
> > - With timer ticks you possibly introduce very little entropy.
> 
> Theoretically, yes. Practically, the improvement to the current state
> of affairs is huge and this has been testified by the test results
> linked in the description. 
> 
> Again, this patch is not about improving the random pattern of the UUID
> values (sorry for the misleading title). It is really about _enabling_
> any kind of randomness in those values at all.
> 
> > - Our random number generator with only 32 state bits remains
> >   sub-standard.
> 
> I believe this is orthogonal to my patch and can be improved
> independently. In addition, whatever is the bit-width of U-Boot PRNG
> (I can find shootouts between various 64/128/256-bit PRNG) its essence
> will not change. It will remain a deterministic number generator,
> whose randomness will still be dictated by the seed.
> 
> > 
> > This is the current situation:
> > 
> > net/bootp.c uses the MAC address to seed the random number generator and
> > uses random numbers for defining waits.
> > 
> > lib/uuid.c is using it for UUID generation.
> 
> I can see that rfc4122 suggests including MAC address in the UUID, but
> it also warns that MAC address could be unavailable:
> 
>  -----------
>    For systems with no IEEE address, a randomly or pseudo-randomly
>    generated value may be used;
>  -----------
> 
> The guess is right. Some SoCs like R-Car3 (in contrast to e.g. i.MX6)
> don't provide any OTP memory/register containing their unique MAC
> address. Needless to say some machines would never connect to IEEE
> network. So, it looks to me that MAC address cannot be taken as
> consistent source of entropy for UUID in U-Boot.

Indeed, we have a lot of platforms where the MAC is not a great source.

> > In the UEFI sub-system I would like to implement the EFI_RNG_PROTOCOL.
> > Linux uses it for randomizing memory layout. iPXE needs it for secure
> > network connections. This requires a good random number generator with
> > sufficient entropy.
> > 
> > We already have implemented a single hardware random number generator in
> > drivers/crypto/ace_sha.c (CONFIG_EXYNOS_ACE_SHA).
> > 
> > Many other CPUs come with a hardware random number generator. In Linux's
> > drivers/char/hw_random/ I found, e.g.
> > 
> > - meson-rng.c (Amlogic)
> > - mtk-rng.c (MediaTek)
> > - st-rng.c (STMicroelectronics)
> > - imx-rng.c (Freescale)
> 
> To the best of my knowledge, there is no HW RNG on R-Car3, so our only
> option is currently U-Boot PRNG.

And we have a lot of systems without HW RNG.

> > I think we should have a u-class for hardware RNGs as one source of entropy.
> > 
> > I would like a random number generator with a high number of state bits
> > (> 127) that we initialize with hardware RNG bits and other sources of
> > entropy. A nice discussion of how Linux does it can be found in [1].
> 
> All sound like future topics to me, orthogonal to this patch.
> Let me know if I misunderstood anything. TIA!

Agreed, this patch sounds like it addresses a number of problems today
that are real problems (I await someone filing a CVE now for our PRNG
problem) and can be iteratively improved on, once merged rather than
having a fundamental problem that needs to be addressed.

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

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Eugeniu Rosca]

Hi Tom,

On Wed, May 01, 2019 at 03:51:49PM -0400, Tom Rini wrote:
[..]
> 
> Agreed, this patch sounds like it addresses a number of problems today
> that are real problems (I await someone filing a CVE now for our PRNG
> problem)

A new CVE has been submitted via https://cveform.mitre.org/.
Will keep this thread posted with any updates from the CVE Team.

> and can be iteratively improved on, once merged rather than
> having a fundamental problem that needs to be addressed.
> 
> -- 
> Tom

--
Best regards,
Eugeniu.

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Eugeniu Rosca]

On Thu, May 02, 2019 at 12:32:53AM +0200, Eugeniu Rosca wrote:
> Hi Tom,
> 
> On Wed, May 01, 2019 at 03:51:49PM -0400, Tom Rini wrote:
> [..]
> > 
> > Agreed, this patch sounds like it addresses a number of problems today
> > that are real problems (I await someone filing a CVE now for our PRNG
> > problem)
> 
> A new CVE has been submitted via https://cveform.mitre.org/.
> Will keep this thread posted with any updates from the CVE Team.

The CVE has been published as:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-11690
https://nvd.nist.gov/vuln/detail/CVE-2019-11690

It looks like it is still WIP.

> 
> > and can be iteratively improved on, once merged rather than
> > having a fundamental problem that needs to be addressed.
> > 
> > -- 
> > Tom
> 
> --
> Best regards,
> Eugeniu.

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Eugeniu Rosca]

On Fri, May 03, 2019 at 06:09:28PM +0200, Eugeniu Rosca wrote:
> On Thu, May 02, 2019 at 12:32:53AM +0200, Eugeniu Rosca wrote:
> > Hi Tom,
> > 
> > On Wed, May 01, 2019 at 03:51:49PM -0400, Tom Rini wrote:
> > [..]
> > > 
> > > Agreed, this patch sounds like it addresses a number of problems today
> > > that are real problems (I await someone filing a CVE now for our PRNG
> > > problem)
> > 
> > A new CVE has been submitted via https://cveform.mitre.org/.
> > Will keep this thread posted with any updates from the CVE Team.
> 
> The CVE has been published as:
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-11690
> https://nvd.nist.gov/vuln/detail/CVE-2019-11690
> 
> It looks like it is still WIP.

jFTR, https://nvd.nist.gov/vuln/detail/CVE-2019-11690 has been populated
with some "Severity and Metrics".

> 
> > 
> > > and can be iteratively improved on, once merged rather than
> > > having a fundamental problem that needs to be addressed.
> > > 
> > > -- 
> > > Tom
> > 
> > --
> > Best regards,
> > Eugeniu.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Matthias Brugger]

On 30/04/2019 21:07, Heinrich Schuchardt wrote:
> On 4/30/19 4:53 AM, 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.
>>
>> As a user, I expect a trully random uuid value in the above example.
>> Otherwise, system/RFS designers and OS people might assume they have
>> a reliable/consistent uuid passed by the bootloader, while the truth
>> is U-Boot simply lacks entropy to generate a random string.
>>
>> In its first attempt [1] to improve the uuid randomness, this patch
>> updated the seed based on the output of get_timer(), similar to [2].
>>
>> 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
>>
>> This second patch addresses both drawbacks.
>>
>> My R-Car3 testing [3] consists of running 'gpt write mmc 1 $partitions'
>> in a loop 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] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")
>> [3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
>>   => while true; do \
>>      env default -a; \
>>      gpt write mmc 1 $partitions; \
>>      print; done
>>
>> Reported-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> This patch may ameliorate the situation for GUIDs a bit. But I dislike:
> 
> - This patch is a uuid only solution to introduce time ticks as source
>   of entropy.
> - With timer ticks you possibly introduce very little entropy.
> - Our random number generator with only 32 state bits remains
>   sub-standard.
> 
> This is the current situation:
> 
> net/bootp.c uses the MAC address to seed the random number generator and
> uses random numbers for defining waits.
> 
> lib/uuid.c is using it for UUID generation.
> 
> In the UEFI sub-system I would like to implement the EFI_RNG_PROTOCOL.
> Linux uses it for randomizing memory layout. iPXE needs it for secure
> network connections. This requires a good random number generator with
> sufficient entropy.
> 
> We already have implemented a single hardware random number generator in
> drivers/crypto/ace_sha.c (CONFIG_EXYNOS_ACE_SHA).
> 
> Many other CPUs come with a hardware random number generator. In Linux's
> drivers/char/hw_random/ I found, e.g.
> 
> - meson-rng.c (Amlogic)
> - mtk-rng.c (MediaTek)
> - st-rng.c (STMicroelectronics)
> - imx-rng.c (Freescale)
> 
> I think we should have a u-class for hardware RNGs as one source of entropy.
> 

Anyone working on this already? If not I can have a look. It could be used for
RPi3 as well (drivers/char/hw_random/bcm2835-rng.c in the Linux kernel). :)

Regards,
Matthias

> I would like a random number generator with a high number of state bits
> (> 127) that we initialize with hardware RNG bits and other sources of
> entropy. A nice discussion of how Linux does it can be found in [1].
> 
> Best regards
> 
> Heinrich
> 
> [1]
> https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf
> 
> 
> 
>> ---
>> v2:
>>   - Replaced get_timer(0) with get_ticks() and added rand() to seed value
>>   - Performed extensive testing on R-Car3 (ARMv8)
>> v1:
>>   - https://patchwork.ozlabs.org/patch/1091802/
>> ---
>>   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 mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 4/4] lib: uuid: Improve randomness of uuid values on RANDOM_UUID=y

[Eugeniu Rosca]

On Thu, May 16, 2019 at 5:14 PM Matthias Brugger <mbrugger@suse.com> wrote:
> On 30/04/2019 21:07, Heinrich Schuchardt wrote:
> >
> > I think we should have a u-class for hardware RNGs as one source of entropy.
>
> Anyone working on this already?

Not me.

> If not I can have a look.

green_light_on(); // :)

> It could be used for
> RPi3 as well (drivers/char/hw_random/bcm2835-rng.c in the Linux kernel). :)
>
> Regards,
> Matthias

-- 
Regards,
Eugeniu.