[U-Boot] [PATCH 1/7] libavb: Handle wrong hashtree_error_mode in avb_append_options()

[U-Boot] [PATCH 1/7] libavb: Handle wrong hashtree_error_mode in avb_append_options()

[Eugeniu Rosca]

From: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>

Exit with AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT
when hashtree_error_mode value passed to avb_append_options()
is unknown (not from AvbHashtreeErrorMode enum).

Otherwise, default value is not handled in the
switch(hashtree_error_mode), which causes below compile warning:

lib/libavb/avb_cmdline.c: In function ‘avb_append_options’:
lib/libavb/avb_cmdline.c:354:13: warning: ‘dm_verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     new_ret = avb_replace(
     ~~~~~~~~^~~~~~~~~~~~~~
         slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode);
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/libavb/avb_cmdline.c:363:8: warning: ‘verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (!cmdline_append_option(
        ^~~~~~~~~~~~~~~~~~~~~~
           slot_data, "androidboot.veritymode", verity_mode)) {

Signed-off-by: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 lib/libavb/avb_cmdline.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c
index 91a6615c740d..d24669927203 100644
--- a/lib/libavb/avb_cmdline.c
+++ b/lib/libavb/avb_cmdline.c
@@ -331,6 +331,9 @@ AvbSlotVerifyResult avb_append_options(
         verity_mode = "logging";
         dm_verity_mode = "ignore_corruption";
         break;
+      default:
+        ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT;
+        goto out;
     }
     new_ret = avb_replace(
         slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode);
-- 
2.18.0

[U-Boot] [PATCH 2/7] common: avb_verify: Fix invalid 'for' loop condition

[Eugeniu Rosca]

Fix below compiler [1] warning:

common/avb_verify.c: In function ‘avb_find_dm_args’:
common/avb_verify.c:179:30: warning: left-hand operand of comma expression has no effect [-Wunused-value]
  for (i = 0; i < AVB_MAX_ARGS, args[i]; ++i) {

[1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/avb_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 20e35ade3029..e6f3f207ff6f 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -176,7 +176,7 @@ static int avb_find_dm_args(char **args, char *str)
 	if (!str)
 		return -1;
 
-	for (i = 0; i < AVB_MAX_ARGS, args[i]; ++i) {
+	for (i = 0; i < AVB_MAX_ARGS && args[i]; ++i) {
 		if (strstr(args[i], str))
 			return i;
 	}
-- 
2.18.0

[U-Boot] [PATCH 3/7] common: kconfig: Mark AVB_VERIFY as dependent on PARTITION_UUIDS

[Eugeniu Rosca]

Avoid below compiler [1] errors, reproduced with configuration [2]:

common/avb_verify.c: In function ‘get_unique_guid_for_partition’:
common/avb_verify.c:692:31: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
  uuid_size = sizeof(part->info.uuid);
                               ^
common/avb_verify.c:696:29: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
  memcpy(guid_buf, part->info.uuid, uuid_size);
                             ^
  LD      drivers/built-in.o
make[2]: *** [scripts/Makefile.build:278: common/avb_verify.o] Error 1

[1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
[2] r8a7795_ulcb_defconfig, plus:
    CONFIG_AVB_VERIFY=y
    CONFIG_PARTITION_UUIDS=y
    CONFIG_UDP_FUNCTION_FASTBOOT=y
    CONFIG_LIBAVB=y

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/Kconfig b/common/Kconfig
index 4d7215a36086..f48888a0e03b 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -640,6 +640,7 @@ config HASH
 config AVB_VERIFY
 	bool "Build Android Verified Boot operations"
 	depends on LIBAVB && FASTBOOT
+	depends on PARTITION_UUIDS
 	help
 	  This option enables compilation of bootloader-dependent operations,
 	  used by Android Verified Boot 2.0 library (libavb). Includes:
-- 
2.18.0

[U-Boot] [PATCH 4/7] common: avb_verify: Make local data static

[Eugeniu Rosca]

Fix sparse complaint:

common/avb_verify.c:14:21: warning: \
  symbol 'avb_root_pub' was not declared. Should it be static?

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/avb_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index e6f3f207ff6f..9c90e1b4ae5c 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -11,7 +11,7 @@
 #include <malloc.h>
 #include <part.h>
 
-const unsigned char avb_root_pub[1032] = {
+static const unsigned char avb_root_pub[1032] = {
 	0x0, 0x0, 0x10, 0x0, 0x55, 0xd9, 0x4, 0xad, 0xd8, 0x4,
 	0xaf, 0xe3, 0xd3, 0x84, 0x6c, 0x7e, 0xd, 0x89, 0x3d, 0xc2,
 	0x8c, 0xd3, 0x12, 0x55, 0xe9, 0x62, 0xc9, 0xf1, 0xf, 0x5e,
-- 
2.18.0

[U-Boot] [PATCH 5/7] common: avb_verify: Fix memory leaks

[Eugeniu Rosca]

Cppcheck (v1.85) reports w/o this patch:

[common/avb_verify.c:351]: (error) Memory leak: part
[common/avb_verify.c:356]: (error) Memory leak: part
[common/avb_verify.c:361]: (error) Memory leak: part
[common/avb_verify.c:366]: (error) Memory leak: part

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/avb_verify.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 9c90e1b4ae5c..58cfa1aa7de8 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -348,34 +348,37 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
 	part->mmc = find_mmc_device(dev_num);
 	if (!part->mmc) {
 		printf("No MMC device at slot %x\n", dev_num);
-		return NULL;
+		goto err;
 	}
 
 	if (mmc_init(part->mmc)) {
 		printf("MMC initialization failed\n");
-		return NULL;
+		goto err;
 	}
 
 	ret = mmc_switch_part(part->mmc, part_num);
 	if (ret)
-		return NULL;
+		goto err;
 
 	mmc_blk = mmc_get_blk_desc(part->mmc);
 	if (!mmc_blk) {
 		printf("Error - failed to obtain block descriptor\n");
-		return NULL;
+		goto err;
 	}
 
 	ret = part_get_info_by_name(mmc_blk, partition, &part->info);
 	if (!ret) {
 		printf("Can't find partition '%s'\n", partition);
-		return NULL;
+		goto err;
 	}
 
 	part->dev_num = dev_num;
 	part->mmc_blk = mmc_blk;
 
 	return part;
+err:
+	free(part);
+	return NULL;
 }
 
 static AvbIOResult mmc_byte_io(AvbOps *ops,
-- 
2.18.0

[U-Boot] [PATCH 6/7] common: avb_verify: Fix never-occurring avb_free(ops_data)

[Eugeniu Rosca]

Cppcheck (v1.85) reports w/o this patch:

[common/avb_verify.c:738] -> [common/avb_verify.c:741]: (warning) \
  Either the condition 'ops' is redundant or there is possible null \
  pointer dereference: ops.

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/avb_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 58cfa1aa7de8..3d2b4cbad92d 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -735,7 +735,7 @@ void avb_ops_free(AvbOps *ops)
 {
 	struct AvbOpsData *ops_data;
 
-	if (ops)
+	if (!ops)
 		return;
 
 	ops_data = ops->user_data;
-- 
2.18.0

[U-Boot] [PATCH 7/7] common: avb_verify: Fix division by zero in mmc_byte_io()

[Eugeniu Rosca]

Compiling U-Boot with ubsan/asan libraries and running it in sandbox
may lead to below backtrace:

 => avb init 0
 => avb verify
 ## Android Verified Boot 2.0 version 1.1.0
read_is_device_unlocked not supported yet
common/avb_verify.c:407:31: runtime error: division by zero
AddressSanitizer:DEADLYSIGNAL
=================================================================
==9388==ERROR: AddressSanitizer: FPE on unknown address 0x0000004b467f \
    (pc 0x0000004b467f bp 0x000000000000 sp 0x7ffd899fe150 T0)
    #0 0x4b467e in mmc_byte_io common/avb_verify.c:407
    #1 0x4b4c47 in mmc_byte_io common/avb_verify.c:532
    #2 0x4b4c47 in read_from_partition common/avb_verify.c:533
    #3 0x69dc0d in load_and_verify_vbmeta lib/libavb/avb_slot_verify.c:560
    #4 0x6a1ee6 in avb_slot_verify lib/libavb/avb_slot_verify.c:1139
    #5 0x45dabd in do_avb_verify_part cmd/avb.c:245
    #6 0x4af77c in cmd_call common/command.c:499
    #7 0x4af77c in cmd_process common/command.c:538
    #8 0x46bafc in run_pipe_real common/cli_hush.c:1677
    #9 0x46bafc in run_list_real common/cli_hush.c:1875
    #10 0x46c780 in run_list common/cli_hush.c:2024
    #11 0x46c780 in parse_stream_outer common/cli_hush.c:3216
    #12 0x46d34b in parse_file_outer common/cli_hush.c:3299
    #13 0x4ad609 in cli_loop common/cli.c:217
    #14 0x4625ae in main_loop common/main.c:65
    #15 0x46f2d1 in run_main_loop common/board_r.c:648
    #16 0x640253 in initcall_run_list lib/initcall.c:30
    #17 0x46f9d0 in board_init_r common/board_r.c:879
    #18 0x40539b in main arch/sandbox/cpu/start.c:321
    #19 0x7fa94925f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #20 0x408908 in _start (/srv/R/u-boot-master/u-boot+0x408908)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE common/avb_verify.c:407 in mmc_byte_io
==9388==ABORTING

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/avb_verify.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 3d2b4cbad92d..759df7bd25c0 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -402,6 +402,9 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
 	if (!part)
 		return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
 
+	if (!part->info.blksz)
+		return AVB_IO_RESULT_ERROR_IO;
+
 	start_offset = calc_offset(part, offset);
 	while (num_bytes) {
 		start_sector = start_offset / part->info.blksz;
-- 
2.18.0

[U-Boot] [PATCH 6/7] common: avb_verify: Fix never-occurring avb_free(ops_data)

[Igor Opaniuk]

Thanks for fixing this!

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Cppcheck (v1.85) reports w/o this patch:
>
> [common/avb_verify.c:738] -> [common/avb_verify.c:741]: (warning) \
>   Either the condition 'ops' is redundant or there is possible null \
>   pointer dereference: ops.
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/avb_verify.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 58cfa1aa7de8..3d2b4cbad92d 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -735,7 +735,7 @@ void avb_ops_free(AvbOps *ops)
>  {
>         struct AvbOpsData *ops_data;
>
> -       if (ops)
> +       if (!ops)
>                 return;
>
>         ops_data = ops->user_data;
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 7/7] common: avb_verify: Fix division by zero in mmc_byte_io()

[Igor Opaniuk]

Thanks for fixing this!

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Compiling U-Boot with ubsan/asan libraries and running it in sandbox
> may lead to below backtrace:
>
>  => avb init 0
>  => avb verify
>  ## Android Verified Boot 2.0 version 1.1.0
> read_is_device_unlocked not supported yet
> common/avb_verify.c:407:31: runtime error: division by zero
> AddressSanitizer:DEADLYSIGNAL
> =================================================================
> ==9388==ERROR: AddressSanitizer: FPE on unknown address 0x0000004b467f \
>     (pc 0x0000004b467f bp 0x000000000000 sp 0x7ffd899fe150 T0)
>     #0 0x4b467e in mmc_byte_io common/avb_verify.c:407
>     #1 0x4b4c47 in mmc_byte_io common/avb_verify.c:532
>     #2 0x4b4c47 in read_from_partition common/avb_verify.c:533
>     #3 0x69dc0d in load_and_verify_vbmeta lib/libavb/avb_slot_verify.c:560
>     #4 0x6a1ee6 in avb_slot_verify lib/libavb/avb_slot_verify.c:1139
>     #5 0x45dabd in do_avb_verify_part cmd/avb.c:245
>     #6 0x4af77c in cmd_call common/command.c:499
>     #7 0x4af77c in cmd_process common/command.c:538
>     #8 0x46bafc in run_pipe_real common/cli_hush.c:1677
>     #9 0x46bafc in run_list_real common/cli_hush.c:1875
>     #10 0x46c780 in run_list common/cli_hush.c:2024
>     #11 0x46c780 in parse_stream_outer common/cli_hush.c:3216
>     #12 0x46d34b in parse_file_outer common/cli_hush.c:3299
>     #13 0x4ad609 in cli_loop common/cli.c:217
>     #14 0x4625ae in main_loop common/main.c:65
>     #15 0x46f2d1 in run_main_loop common/board_r.c:648
>     #16 0x640253 in initcall_run_list lib/initcall.c:30
>     #17 0x46f9d0 in board_init_r common/board_r.c:879
>     #18 0x40539b in main arch/sandbox/cpu/start.c:321
>     #19 0x7fa94925f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
>     #20 0x408908 in _start (/srv/R/u-boot-master/u-boot+0x408908)
>
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: FPE common/avb_verify.c:407 in mmc_byte_io
> ==9388==ABORTING
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/avb_verify.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 3d2b4cbad92d..759df7bd25c0 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -402,6 +402,9 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
>         if (!part)
>                 return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
>
> +       if (!part->info.blksz)
> +               return AVB_IO_RESULT_ERROR_IO;
> +
>         start_offset = calc_offset(part, offset);
>         while (num_bytes) {
>                 start_sector = start_offset / part->info.blksz;
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 1/7] libavb: Handle wrong hashtree_error_mode in avb_append_options()

[Igor Opaniuk]

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> From: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>
>
> Exit with AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT
> when hashtree_error_mode value passed to avb_append_options()
> is unknown (not from AvbHashtreeErrorMode enum).
>
> Otherwise, default value is not handled in the
> switch(hashtree_error_mode), which causes below compile warning:
>
> lib/libavb/avb_cmdline.c: In function ‘avb_append_options’:
> lib/libavb/avb_cmdline.c:354:13: warning: ‘dm_verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>      new_ret = avb_replace(
>      ~~~~~~~~^~~~~~~~~~~~~~
>          slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode);
>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/libavb/avb_cmdline.c:363:8: warning: ‘verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (!cmdline_append_option(
>         ^~~~~~~~~~~~~~~~~~~~~~
>            slot_data, "androidboot.veritymode", verity_mode)) {
>
> Signed-off-by: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  lib/libavb/avb_cmdline.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c
> index 91a6615c740d..d24669927203 100644
> --- a/lib/libavb/avb_cmdline.c
> +++ b/lib/libavb/avb_cmdline.c
> @@ -331,6 +331,9 @@ AvbSlotVerifyResult avb_append_options(
>          verity_mode = "logging";
>          dm_verity_mode = "ignore_corruption";
>          break;
> +      default:
> +        ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT;
> +        goto out;
>      }
>      new_ret = avb_replace(
>          slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode);
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 2/7] common: avb_verify: Fix invalid 'for' loop condition

[Igor Opaniuk]

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Fix below compiler [1] warning:
>
> common/avb_verify.c: In function ‘avb_find_dm_args’:
> common/avb_verify.c:179:30: warning: left-hand operand of comma expression has no effect [-Wunused-value]
>   for (i = 0; i < AVB_MAX_ARGS, args[i]; ++i) {
>
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/avb_verify.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 20e35ade3029..e6f3f207ff6f 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -176,7 +176,7 @@ static int avb_find_dm_args(char **args, char *str)
>         if (!str)
>                 return -1;
>
> -       for (i = 0; i < AVB_MAX_ARGS, args[i]; ++i) {
> +       for (i = 0; i < AVB_MAX_ARGS && args[i]; ++i) {
>                 if (strstr(args[i], str))
>                         return i;
>         }
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 3/7] common: kconfig: Mark AVB_VERIFY as dependent on PARTITION_UUIDS

[Igor Opaniuk]

Hi Eugeniu,

Why not keep all dependencies on the same line in this case? Simply:
depends LIBAVB && FASTBOOT && PARTITION_UUIDS

Regards,
Igor

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Avoid below compiler [1] errors, reproduced with configuration [2]:
>
> common/avb_verify.c: In function ‘get_unique_guid_for_partition’:
> common/avb_verify.c:692:31: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
>   uuid_size = sizeof(part->info.uuid);
>                                ^
> common/avb_verify.c:696:29: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
>   memcpy(guid_buf, part->info.uuid, uuid_size);
>                              ^
>   LD      drivers/built-in.o
> make[2]: *** [scripts/Makefile.build:278: common/avb_verify.o] Error 1
>
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
> [2] r8a7795_ulcb_defconfig, plus:
>     CONFIG_AVB_VERIFY=y
>     CONFIG_PARTITION_UUIDS=y
>     CONFIG_UDP_FUNCTION_FASTBOOT=y
>     CONFIG_LIBAVB=y
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/common/Kconfig b/common/Kconfig
> index 4d7215a36086..f48888a0e03b 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -640,6 +640,7 @@ config HASH
>  config AVB_VERIFY
>         bool "Build Android Verified Boot operations"
>         depends on LIBAVB && FASTBOOT
> +       depends on PARTITION_UUIDS
>         help
>           This option enables compilation of bootloader-dependent operations,
>           used by Android Verified Boot 2.0 library (libavb). Includes:
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 4/7] common: avb_verify: Make local data static

[Igor Opaniuk]

Hi Eugeniu,
thanks for fixing this!

BTW, I plan to replace this workaround with hardcoded public RSA key
for verification of vbmeta image signature,
and an option to store it in RPMB using AVB TA/OP-TEE instead, just
after Jens's Wiklander patches (that he sent last Friday) are merged.

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Fix sparse complaint:
>
> common/avb_verify.c:14:21: warning: \
>   symbol 'avb_root_pub' was not declared. Should it be static?
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/avb_verify.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index e6f3f207ff6f..9c90e1b4ae5c 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -11,7 +11,7 @@
>  #include <malloc.h>
>  #include <part.h>
>
> -const unsigned char avb_root_pub[1032] = {
> +static const unsigned char avb_root_pub[1032] = {
>         0x0, 0x0, 0x10, 0x0, 0x55, 0xd9, 0x4, 0xad, 0xd8, 0x4,
>         0xaf, 0xe3, 0xd3, 0x84, 0x6c, 0x7e, 0xd, 0x89, 0x3d, 0xc2,
>         0x8c, 0xd3, 0x12, 0x55, 0xe9, 0x62, 0xc9, 0xf1, 0xf, 0x5e,
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 5/7] common: avb_verify: Fix memory leaks

[Igor Opaniuk]

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 14 August 2018 at 03:43, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Cppcheck (v1.85) reports w/o this patch:
>
> [common/avb_verify.c:351]: (error) Memory leak: part
> [common/avb_verify.c:356]: (error) Memory leak: part
> [common/avb_verify.c:361]: (error) Memory leak: part
> [common/avb_verify.c:366]: (error) Memory leak: part
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/avb_verify.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 9c90e1b4ae5c..58cfa1aa7de8 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -348,34 +348,37 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
>         part->mmc = find_mmc_device(dev_num);
>         if (!part->mmc) {
>                 printf("No MMC device at slot %x\n", dev_num);
> -               return NULL;
> +               goto err;
>         }
>
>         if (mmc_init(part->mmc)) {
>                 printf("MMC initialization failed\n");
> -               return NULL;
> +               goto err;
>         }
>
>         ret = mmc_switch_part(part->mmc, part_num);
>         if (ret)
> -               return NULL;
> +               goto err;
>
>         mmc_blk = mmc_get_blk_desc(part->mmc);
>         if (!mmc_blk) {
>                 printf("Error - failed to obtain block descriptor\n");
> -               return NULL;
> +               goto err;
>         }
>
>         ret = part_get_info_by_name(mmc_blk, partition, &part->info);
>         if (!ret) {
>                 printf("Can't find partition '%s'\n", partition);
> -               return NULL;
> +               goto err;
>         }
>
>         part->dev_num = dev_num;
>         part->mmc_blk = mmc_blk;
>
>         return part;
> +err:
> +       free(part);
> +       return NULL;
>  }
>
>  static AvbIOResult mmc_byte_io(AvbOps *ops,
> --
> 2.18.0
>



-- 
Regards,
Igor Opaniuk

[U-Boot] [PATCH 3/7] common: kconfig: Mark AVB_VERIFY as dependent on PARTITION_UUIDS

[Eugeniu Rosca]

Hi Igor,

First, thanks for the reviews!

On Thu, Aug 16, 2018 at 11:38:18AM +0300, Igor Opaniuk wrote:
> Hi Eugeniu,
> 
> Why not keep all dependencies on the same line in this case? Simply:
> depends LIBAVB && FASTBOOT && PARTITION_UUIDS

I guess it's a matter of personal preference (but maybe not entirely).
Let's say one needs to replace "depends on PARTITION_UUIDS" with
"select PARTITION_UUIDS" due to e.g. a circular dependency detected
by Kconfig at some point. For me below two lines:

- depends on PARTITION_UUIDS
+ select PARTITION_UUIDS

look more readable and are easier to review than:

- depends LIBAVB && FASTBOOT && PARTITION_UUIDS
+ depends LIBAVB && FASTBOOT
+ select PARTITION_UUIDS

I still can update the patch. Just let me know.

> 
> Regards,
> Igor

Best regards,
Eugeniu.

[U-Boot] [PATCH 4/7] common: avb_verify: Make local data static

[Eugeniu Rosca]

On Thu, Aug 16, 2018 at 12:07:25PM +0300, Igor Opaniuk wrote:
> Hi Eugeniu,
> thanks for fixing this!
> 
> BTW, I plan to replace this workaround with hardcoded public RSA key
> for verification of vbmeta image signature,
> and an option to store it in RPMB using AVB TA/OP-TEE instead, just
> after Jens's Wiklander patches (that he sent last Friday) are merged.

Sounds really good!
Please, feel free to NAK this patch given its very short lifetime.

> 
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Thanks,
Eugeniu.

[U-Boot] [PATCH 3/7] common: kconfig: Mark AVB_VERIFY as dependent on PARTITION_UUIDS

[Igor Opaniuk]

Hi Eugeniu,

Makes sense, thanks for the explanation.

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

On 16 August 2018 at 21:25, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Hi Igor,
>
> First, thanks for the reviews!
>
> On Thu, Aug 16, 2018 at 11:38:18AM +0300, Igor Opaniuk wrote:
>> Hi Eugeniu,
>>
>> Why not keep all dependencies on the same line in this case? Simply:
>> depends LIBAVB && FASTBOOT && PARTITION_UUIDS
>
> I guess it's a matter of personal preference (but maybe not entirely).
> Let's say one needs to replace "depends on PARTITION_UUIDS" with
> "select PARTITION_UUIDS" due to e.g. a circular dependency detected
> by Kconfig at some point. For me below two lines:
>
> - depends on PARTITION_UUIDS
> + select PARTITION_UUIDS
>
> look more readable and are easier to review than:
>
> - depends LIBAVB && FASTBOOT && PARTITION_UUIDS
> + depends LIBAVB && FASTBOOT
> + select PARTITION_UUIDS
>
> I still can update the patch. Just let me know.
>
>>
>> Regards,
>> Igor
>
> Best regards,
> Eugeniu.



-- 
Regards,
Igor Opaniuk

[U-Boot] [U-Boot, 1/7] libavb: Handle wrong hashtree_error_mode in avb_append_options()

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:03AM +0200, Eugeniu Rosca wrote:

> From: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>
> 
> Exit with AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT
> when hashtree_error_mode value passed to avb_append_options()
> is unknown (not from AvbHashtreeErrorMode enum).
> 
> Otherwise, default value is not handled in the
> switch(hashtree_error_mode), which causes below compile warning:
> 
> lib/libavb/avb_cmdline.c: In function ‘avb_append_options’:
> lib/libavb/avb_cmdline.c:354:13: warning: ‘dm_verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>      new_ret = avb_replace(
>      ~~~~~~~~^~~~~~~~~~~~~~
>          slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode);
>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/libavb/avb_cmdline.c:363:8: warning: ‘verity_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (!cmdline_append_option(
>         ^~~~~~~~~~~~~~~~~~~~~~
>            slot_data, "androidboot.veritymode", verity_mode)) {
> 
> Signed-off-by: Ievgen Maliarenko <ievgen.maliarenko@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 2/7] common: avb_verify: Fix invalid 'for' loop condition

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:04AM +0200, Eugeniu Rosca wrote:

> Fix below compiler [1] warning:
> 
> common/avb_verify.c: In function ‘avb_find_dm_args’:
> common/avb_verify.c:179:30: warning: left-hand operand of comma expression has no effect [-Wunused-value]
>   for (i = 0; i < AVB_MAX_ARGS, args[i]; ++i) {
> 
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 3/7] common: kconfig: Mark AVB_VERIFY as dependent on PARTITION_UUIDS

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:05AM +0200, Eugeniu Rosca wrote:

> Avoid below compiler [1] errors, reproduced with configuration [2]:
> 
> common/avb_verify.c: In function ‘get_unique_guid_for_partition’:
> common/avb_verify.c:692:31: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
>   uuid_size = sizeof(part->info.uuid);
>                                ^
> common/avb_verify.c:696:29: error: ‘disk_partition_t {aka struct disk_partition}’ has no member named ‘uuid’
>   memcpy(guid_buf, part->info.uuid, uuid_size);
>                              ^
>   LD      drivers/built-in.o
> make[2]: *** [scripts/Makefile.build:278: common/avb_verify.o] Error 1
> 
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
> [2] r8a7795_ulcb_defconfig, plus:
>     CONFIG_AVB_VERIFY=y
>     CONFIG_PARTITION_UUIDS=y
>     CONFIG_UDP_FUNCTION_FASTBOOT=y
>     CONFIG_LIBAVB=y
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 4/7] common: avb_verify: Make local data static

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:06AM +0200, Eugeniu Rosca wrote:

> Fix sparse complaint:
> 
> common/avb_verify.c:14:21: warning: \
>   symbol 'avb_root_pub' was not declared. Should it be static?
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,5/7] common: avb_verify: Fix memory leaks

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:07AM +0200, Eugeniu Rosca wrote:

> Cppcheck (v1.85) reports w/o this patch:
> 
> [common/avb_verify.c:351]: (error) Memory leak: part
> [common/avb_verify.c:356]: (error) Memory leak: part
> [common/avb_verify.c:361]: (error) Memory leak: part
> [common/avb_verify.c:366]: (error) Memory leak: part
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 6/7] common: avb_verify: Fix never-occurring avb_free(ops_data)

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:08AM +0200, Eugeniu Rosca wrote:

> Cppcheck (v1.85) reports w/o this patch:
> 
> [common/avb_verify.c:738] -> [common/avb_verify.c:741]: (warning) \
>   Either the condition 'ops' is redundant or there is possible null \
>   pointer dereference: ops.
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 7/7] common: avb_verify: Fix division by zero in mmc_byte_io()

[Tom Rini]

On Tue, Aug 14, 2018 at 02:43:09AM +0200, Eugeniu Rosca wrote:

> Compiling U-Boot with ubsan/asan libraries and running it in sandbox
> may lead to below backtrace:
> 
>  => avb init 0
>  => avb verify
>  ## Android Verified Boot 2.0 version 1.1.0
> read_is_device_unlocked not supported yet
> common/avb_verify.c:407:31: runtime error: division by zero
> AddressSanitizer:DEADLYSIGNAL
> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>
> 
> =================================================================
> ==9388==ERROR: AddressSanitizer: FPE on unknown address 0x0000004b467f \
>     (pc 0x0000004b467f bp 0x000000000000 sp 0x7ffd899fe150 T0)
>     #0 0x4b467e in mmc_byte_io common/avb_verify.c:407
>     #1 0x4b4c47 in mmc_byte_io common/avb_verify.c:532
>     #2 0x4b4c47 in read_from_partition common/avb_verify.c:533
>     #3 0x69dc0d in load_and_verify_vbmeta lib/libavb/avb_slot_verify.c:560
>     #4 0x6a1ee6 in avb_slot_verify lib/libavb/avb_slot_verify.c:1139
>     #5 0x45dabd in do_avb_verify_part cmd/avb.c:245
>     #6 0x4af77c in cmd_call common/command.c:499
>     #7 0x4af77c in cmd_process common/command.c:538
>     #8 0x46bafc in run_pipe_real common/cli_hush.c:1677
>     #9 0x46bafc in run_list_real common/cli_hush.c:1875
>     #10 0x46c780 in run_list common/cli_hush.c:2024
>     #11 0x46c780 in parse_stream_outer common/cli_hush.c:3216
>     #12 0x46d34b in parse_file_outer common/cli_hush.c:3299
>     #13 0x4ad609 in cli_loop common/cli.c:217
>     #14 0x4625ae in main_loop common/main.c:65
>     #15 0x46f2d1 in run_main_loop common/board_r.c:648
>     #16 0x640253 in initcall_run_list lib/initcall.c:30
>     #17 0x46f9d0 in board_init_r common/board_r.c:879
>     #18 0x40539b in main arch/sandbox/cpu/start.c:321
>     #19 0x7fa94925f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
>     #20 0x408908 in _start (/srv/R/u-boot-master/u-boot+0x408908)
> 
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: FPE common/avb_verify.c:407 in mmc_byte_io
> ==9388==ABORTING
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Applied to u-boot/master, thanks!

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