[PATCH v1 0/7] AVB: cosmetic adjustments/improvements

[PATCH v1 0/7] AVB: cosmetic adjustments/improvements

[Igor Opaniuk]

This patch series include:
- Simplify and add more context for debug/error prints where it's needed.
- Move SPDX license identifiers to the first line, so it conforms
  to license placement rule.
- Use mmc_switch_part() only for eMMC.
- Rework do_avb_verify_part, take into account device lock state for setting correct
  androidboot.verifiedbootstate.
- Adjust AVB documentation, add info about slot_suffix param.

Igor Opaniuk (7):
  common: avb_verify: don't call mmc_switch_part for SD
  avb: move SPDX license itdentifiers to the first line
  common: avb_verify: rework error/debug prints
  cmd: avb: rework prints
  common: avb_verify: add str_avb_io_error/str_avb_slot_error
  cmd: avb: rework do_avb_verify_part
  doc: android: avb: add slot_suffix param details

 cmd/avb.c                              | 206 ++++++++++++-------------
 common/avb_verify.c                    |  89 +++++++++--
 doc/android/avb2.rst                   |   8 +-
 include/avb_verify.h                   |   7 +-
 test/py/tests/test_android/test_avb.py |   3 +-
 5 files changed, 177 insertions(+), 136 deletions(-)

-- 
2.34.1

[PATCH v1 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

mmc_switch_part() is used for switching between hw partitions
on eMMC (boot0, boot1, user, rpmb).
There is no need to do that for SD card.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 common/avb_verify.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 48ba8db51e5..59f2c25e0de 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -358,9 +358,11 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
 		goto err;
 	}
 
-	ret = mmc_switch_part(part->mmc, part_num);
-	if (ret)
-		goto err;
+	if (IS_MMC(part->mmc)) {
+		ret = mmc_switch_part(part->mmc, part_num);
+		if (ret)
+			goto err;
+	}
 
 	mmc_blk = mmc_get_blk_desc(part->mmc);
 	if (!mmc_blk) {
-- 
2.34.1

Re: [PATCH v1 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> mmc_switch_part() is used for switching between hw partitions
> on eMMC (boot0, boot1, user, rpmb).
> There is no need to do that for SD card.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
>  common/avb_verify.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 48ba8db51e5..59f2c25e0de 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -358,9 +358,11 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
>  		goto err;
>  	}
>  
> -	ret = mmc_switch_part(part->mmc, part_num);
> -	if (ret)
> -		goto err;
> +	if (IS_MMC(part->mmc)) {
> +		ret = mmc_switch_part(part->mmc, part_num);
> +		if (ret)
> +			goto err;
> +	}
>  
>  	mmc_blk = mmc_get_blk_desc(part->mmc);
>  	if (!mmc_blk) {
> -- 
> 2.34.1

[PATCH v1 2/7] avb: move SPDX license itdentifiers to the first line

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Move SPDX license identifiers to the first line, so it conforms
to license placement rule [1]:

Placement:
The SPDX license identifier in kernel files shall be added at the first
possible line in a file which can contain a comment.  For the majority
of files this is the first line, except for scripts which require the
'#!PATH_TO_INTERPRETER' in the first line.  For those scripts the SPDX
identifier goes into the second line.

[1] https://www.kernel.org/doc/Documentation/process/license-rules.rst

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 cmd/avb.c                              | 4 +---
 common/avb_verify.c                    | 3 +--
 include/avb_verify.h                   | 4 +---
 test/py/tests/test_android/test_avb.py | 3 +--
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index 783f51b8169..ce8b63873f2 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -1,8 +1,6 @@
-
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #include <avb_verify.h>
diff --git a/common/avb_verify.c b/common/avb_verify.c
index 59f2c25e0de..938a5383b5d 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -1,7 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #include <avb_verify.h>
diff --git a/include/avb_verify.h b/include/avb_verify.h
index 1e787ba6668..2fb850044d9 100644
--- a/include/avb_verify.h
+++ b/include/avb_verify.h
@@ -1,8 +1,6 @@
-
+/* SPDX-License-Identifier: GPL-2.0+ */
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #ifndef	_AVB_VERIFY_H
diff --git a/test/py/tests/test_android/test_avb.py b/test/py/tests/test_android/test_avb.py
index 238b48c90fa..865efbca4de 100644
--- a/test/py/tests/test_android/test_avb.py
+++ b/test/py/tests/test_android/test_avb.py
@@ -1,6 +1,5 @@
-# Copyright (c) 2018, Linaro Limited
-#
 # SPDX-License-Identifier:  GPL-2.0+
+# Copyright (c) 2018, Linaro Limited
 #
 # Android Verified Boot 2.0 Test
 
-- 
2.34.1

Re: [PATCH v1 2/7] avb: move SPDX license itdentifiers to the first line

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Move SPDX license identifiers to the first line, so it conforms
> to license placement rule [1]:
>
> Placement:
> The SPDX license identifier in kernel files shall be added at the first
> possible line in a file which can contain a comment.  For the majority
> of files this is the first line, except for scripts which require the
> '#!PATH_TO_INTERPRETER' in the first line.  For those scripts the SPDX
> identifier goes into the second line.
>
> [1] https://www.kernel.org/doc/Documentation/process/license-rules.rst
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Nitpick in the title: itdentifiers -> identifiers.
If no other remarks in the series, will fix when applying.

> ---
>
>  cmd/avb.c                              | 4 +---
>  common/avb_verify.c                    | 3 +--
>  include/avb_verify.h                   | 4 +---
>  test/py/tests/test_android/test_avb.py | 3 +--
>  4 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index 783f51b8169..ce8b63873f2 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -1,8 +1,6 @@
> -
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
>   * (C) Copyright 2018, Linaro Limited
> - *
> - * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
>  #include <avb_verify.h>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 59f2c25e0de..938a5383b5d 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -1,7 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
>   * (C) Copyright 2018, Linaro Limited
> - *
> - * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
>  #include <avb_verify.h>
> diff --git a/include/avb_verify.h b/include/avb_verify.h
> index 1e787ba6668..2fb850044d9 100644
> --- a/include/avb_verify.h
> +++ b/include/avb_verify.h
> @@ -1,8 +1,6 @@
> -
> +/* SPDX-License-Identifier: GPL-2.0+ */
>  /*
>   * (C) Copyright 2018, Linaro Limited
> - *
> - * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
>  #ifndef	_AVB_VERIFY_H
> diff --git a/test/py/tests/test_android/test_avb.py b/test/py/tests/test_android/test_avb.py
> index 238b48c90fa..865efbca4de 100644
> --- a/test/py/tests/test_android/test_avb.py
> +++ b/test/py/tests/test_android/test_avb.py
> @@ -1,6 +1,5 @@
> -# Copyright (c) 2018, Linaro Limited
> -#
>  # SPDX-License-Identifier:  GPL-2.0+
> +# Copyright (c) 2018, Linaro Limited
>  #
>  # Android Verified Boot 2.0 Test
>  
> -- 
> 2.34.1

[PATCH v1 3/7] common: avb_verify: rework error/debug prints

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Make error prints more verbose with additional context.
Also s/print/debug/g for prints, which might be relevant only
for debugging purposes.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 common/avb_verify.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 938a5383b5d..ed58239cf8a 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -279,9 +279,9 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part,
 	 * Reading fails on unaligned buffers, so we have to
 	 * use aligned temporary buffer and then copy to destination
 	 */
-
 	if (unaligned) {
-		printf("Handling unaligned read buffer..\n");
+		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
+		      __func__, buffer);
 		tmp_buf = get_sector_buf();
 		buf_size = get_sector_buf_size();
 		if (sectors > buf_size / part->info.blksz)
@@ -320,7 +320,8 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start,
 	if (unaligned) {
 		tmp_buf = get_sector_buf();
 		buf_size = get_sector_buf_size();
-		printf("Handling unaligned wrire buffer..\n");
+		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
+		      __func__, buffer);
 		if (sectors > buf_size / part->info.blksz)
 			sectors = buf_size / part->info.blksz;
 
@@ -348,30 +349,35 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
 	dev_num = get_boot_device(ops);
 	part->mmc = find_mmc_device(dev_num);
 	if (!part->mmc) {
-		printf("No MMC device at slot %x\n", dev_num);
+		printf("%s: no MMC device at slot %x\n", __func__, dev_num);
 		goto err;
 	}
 
-	if (mmc_init(part->mmc)) {
-		printf("MMC initialization failed\n");
+	ret = mmc_init(part->mmc);
+	if (ret) {
+		printf("%s: MMC initialization failed, err = %d\n",
+		       __func__, ret);
 		goto err;
 	}
 
 	if (IS_MMC(part->mmc)) {
 		ret = mmc_switch_part(part->mmc, part_num);
-		if (ret)
+		if (ret) {
+			printf("%s: MMC part switch failed, err = %d\n",
+			       __func__, ret);
 			goto err;
+		}
 	}
 
 	mmc_blk = mmc_get_blk_desc(part->mmc);
 	if (!mmc_blk) {
-		printf("Error - failed to obtain block descriptor\n");
+		printf("%s: failed to obtain block descriptor\n", __func__);
 		goto err;
 	}
 
 	ret = part_get_info_by_name(mmc_blk, partition, &part->info);
 	if (ret < 0) {
-		printf("Can't find partition '%s'\n", partition);
+		printf("%s: can't find partition '%s'\n", __func__, partition);
 		goto err;
 	}
 
@@ -684,7 +690,7 @@ static AvbIOResult read_rollback_index(AvbOps *ops,
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now we always return 0 as the stored rollback index. */
-	printf("%s not supported yet\n", __func__);
+	debug("%s: rollback protection is not implemented\n", __func__);
 
 	if (out_rollback_index)
 		*out_rollback_index = 0;
@@ -730,7 +736,7 @@ static AvbIOResult write_rollback_index(AvbOps *ops,
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now this is a no-op. */
-	printf("%s not supported yet\n", __func__);
+	debug("%s: rollback protection is not implemented\n", __func__);
 
 	return AVB_IO_RESULT_OK;
 #else
@@ -766,8 +772,7 @@ static AvbIOResult read_is_device_unlocked(AvbOps *ops, bool *out_is_unlocked)
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now we always return that the device is unlocked. */
-
-	printf("%s not supported yet\n", __func__);
+	debug("%s: device locking is not implemented\n", __func__);
 
 	*out_is_unlocked = true;
 
-- 
2.34.1

Re: [PATCH v1 3/7] common: avb_verify: rework error/debug prints

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Make error prints more verbose with additional context.
> Also s/print/debug/g for prints, which might be relevant only
> for debugging purposes.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
>  common/avb_verify.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 938a5383b5d..ed58239cf8a 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -279,9 +279,9 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part,
>  	 * Reading fails on unaligned buffers, so we have to
>  	 * use aligned temporary buffer and then copy to destination
>  	 */
> -
>  	if (unaligned) {
> -		printf("Handling unaligned read buffer..\n");
> +		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
> +		      __func__, buffer);
>  		tmp_buf = get_sector_buf();
>  		buf_size = get_sector_buf_size();
>  		if (sectors > buf_size / part->info.blksz)
> @@ -320,7 +320,8 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start,
>  	if (unaligned) {
>  		tmp_buf = get_sector_buf();
>  		buf_size = get_sector_buf_size();
> -		printf("Handling unaligned wrire buffer..\n");
> +		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
> +		      __func__, buffer);
>  		if (sectors > buf_size / part->info.blksz)
>  			sectors = buf_size / part->info.blksz;
>  
> @@ -348,30 +349,35 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
>  	dev_num = get_boot_device(ops);
>  	part->mmc = find_mmc_device(dev_num);
>  	if (!part->mmc) {
> -		printf("No MMC device at slot %x\n", dev_num);
> +		printf("%s: no MMC device at slot %x\n", __func__, dev_num);
>  		goto err;
>  	}
>  
> -	if (mmc_init(part->mmc)) {
> -		printf("MMC initialization failed\n");
> +	ret = mmc_init(part->mmc);
> +	if (ret) {
> +		printf("%s: MMC initialization failed, err = %d\n",
> +		       __func__, ret);
>  		goto err;
>  	}
>  
>  	if (IS_MMC(part->mmc)) {
>  		ret = mmc_switch_part(part->mmc, part_num);
> -		if (ret)
> +		if (ret) {
> +			printf("%s: MMC part switch failed, err = %d\n",
> +			       __func__, ret);
>  			goto err;
> +		}
>  	}
>  
>  	mmc_blk = mmc_get_blk_desc(part->mmc);
>  	if (!mmc_blk) {
> -		printf("Error - failed to obtain block descriptor\n");
> +		printf("%s: failed to obtain block descriptor\n", __func__);
>  		goto err;
>  	}
>  
>  	ret = part_get_info_by_name(mmc_blk, partition, &part->info);
>  	if (ret < 0) {
> -		printf("Can't find partition '%s'\n", partition);
> +		printf("%s: can't find partition '%s'\n", __func__, partition);
>  		goto err;
>  	}
>  
> @@ -684,7 +690,7 @@ static AvbIOResult read_rollback_index(AvbOps *ops,
>  {
>  #ifndef CONFIG_OPTEE_TA_AVB
>  	/* For now we always return 0 as the stored rollback index. */
> -	printf("%s not supported yet\n", __func__);
> +	debug("%s: rollback protection is not implemented\n", __func__);
>  
>  	if (out_rollback_index)
>  		*out_rollback_index = 0;
> @@ -730,7 +736,7 @@ static AvbIOResult write_rollback_index(AvbOps *ops,
>  {
>  #ifndef CONFIG_OPTEE_TA_AVB
>  	/* For now this is a no-op. */
> -	printf("%s not supported yet\n", __func__);
> +	debug("%s: rollback protection is not implemented\n", __func__);
>  
>  	return AVB_IO_RESULT_OK;
>  #else
> @@ -766,8 +772,7 @@ static AvbIOResult read_is_device_unlocked(AvbOps *ops, bool *out_is_unlocked)
>  {
>  #ifndef CONFIG_OPTEE_TA_AVB
>  	/* For now we always return that the device is unlocked. */
> -
> -	printf("%s not supported yet\n", __func__);
> +	debug("%s: device locking is not implemented\n", __func__);
>  
>  	*out_is_unlocked = true;
>  
> -- 
> 2.34.1

[PATCH v1 4/7] cmd: avb: rework prints

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Introduce AVB_OPS_CHECK macro for checking AvbOps before using
it to avoid code duplication.
Simplify and add more context for prints where it's needed.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 cmd/avb.c | 156 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 80 insertions(+), 76 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index ce8b63873f2..ae0012c0e79 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -11,6 +11,14 @@
 #include <mmc.h>
 
 #define AVB_BOOTARGS	"avb_bootargs"
+
+#define AVB_OPS_CHECK(avb_ops) do { \
+	if (!(avb_ops)) { \
+		printf("AVB is not initialized, please run 'avb init <id>'\n"); \
+		return CMD_RET_FAILURE; \
+	} \
+} while (false)
+
 static struct AvbOps *avb_ops;
 
 int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
@@ -28,8 +36,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	avb_ops = avb_ops_alloc(mmc_dev);
 	if (avb_ops)
 		return CMD_RET_SUCCESS;
+	else
+		printf("Can't allocate AvbOps");
 
-	printf("Failed to initialize avb2\n");
+	printf("Failed to initialize AVB\n");
 
 	return CMD_RET_FAILURE;
 }
@@ -41,11 +51,9 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes, bytes_read = 0;
 	void *buffer;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
-		return CMD_RET_USAGE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 5)
 		return CMD_RET_USAGE;
@@ -55,14 +63,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	bytes = hextoul(argv[3], NULL);
 	buffer = (void *)hextoul(argv[4], NULL);
 
-	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
-					 buffer, &bytes_read) ==
-					 AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_from_partition(avb_ops, part, offset,
+					   bytes, buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Read %zu bytes\n", bytes_read);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read from partition\n");
+	printf("Failed to read from partition '%s', err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -74,11 +83,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes, bytes_read = 0;
 	char *buffer;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
-		return CMD_RET_USAGE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 4)
 		return CMD_RET_USAGE;
@@ -94,8 +101,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 	}
 	memset(buffer, 0, bytes);
 
-	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
-					 &bytes_read) == AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_from_partition(avb_ops, part, offset,
+					   bytes, buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
 		printf("Data: ");
 		for (int i = 0; i < bytes_read; i++)
@@ -107,7 +115,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read from partition\n");
+	printf("Failed to read from partition '%s', err = %d\n",
+	       part, ret);
 
 	free(buffer);
 	return CMD_RET_FAILURE;
@@ -120,11 +129,9 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes;
 	void *buffer;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 5)
 		return CMD_RET_USAGE;
@@ -134,13 +141,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	bytes = hextoul(argv[3], NULL);
 	buffer = (void *)hextoul(argv[4], NULL);
 
-	if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->write_to_partition(avb_ops, part, offset,
+					  bytes, buffer);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Wrote %zu bytes\n", bytes);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to write in partition\n");
+	printf("Failed to write in partition '%s', err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -150,24 +159,23 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	size_t index;
 	u64 rb_idx;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 2)
 		return CMD_RET_USAGE;
 
 	index = (size_t)hextoul(argv[1], NULL);
 
-	if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Rollback index: %llx\n", rb_idx);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read rollback index\n");
+	printf("Failed to read rollback index id = %zu, err = %d\n",
+	       index, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -177,11 +185,9 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	size_t index;
 	u64 rb_idx;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 3)
 		return CMD_RET_USAGE;
@@ -189,11 +195,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 	index = (size_t)hextoul(argv[1], NULL);
 	rb_idx = hextoul(argv[2], NULL);
 
-	if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
-	    AVB_IO_RESULT_OK)
+	ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx);
+	if (ret == AVB_IO_RESULT_OK)
 		return CMD_RET_SUCCESS;
 
-	printf("Failed to write rollback index\n");
+	printf("Failed to write rollback index id = %zu, err = %d\n",
+	       index, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -203,25 +210,25 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
 {
 	const char *part;
 	char buffer[UUID_STR_LEN + 1];
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 2)
 		return CMD_RET_USAGE;
 
 	part = argv[1];
 
-	if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
-						   UUID_STR_LEN + 1) ==
-						   AVB_IO_RESULT_OK) {
+	ret = avb_ops->get_unique_guid_for_partition(avb_ops, part,
+						     buffer,
+						     UUID_STR_LEN + 1);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("'%s' UUID: %s\n", part, buffer);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read UUID\n");
+	printf("Failed to read partition '%s' UUID, err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -235,14 +242,12 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	char *cmdline;
 	char *extra_args;
 	char *slot_suffix = "";
+	int ret;
 
 	bool unlocked = false;
 	int res = CMD_RET_FAILURE;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc < 1 || argc > 2)
 		return CMD_RET_USAGE;
@@ -253,9 +258,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	printf("## Android Verified Boot 2.0 version %s\n",
 	       avb_version_string());
 
-	if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
-	    AVB_IO_RESULT_OK) {
-		printf("Can't determine device lock state.\n");
+	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
+	if (ret != AVB_IO_RESULT_OK) {
+		printf("Can't determine device lock state, err = %d\n",
+		       ret);
 		return CMD_RET_FAILURE;
 	}
 
@@ -302,10 +308,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 		printf("Corrupted dm-verity metadata detected\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
-		printf("Unsupported version avbtool was used\n");
+		printf("Unsupported version of avbtool was used\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
-		printf("Checking rollback index failed\n");
+		printf("Rollback index check failed\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
 		printf("Public key was rejected\n");
@@ -324,24 +330,23 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
 		       int argc, char *const argv[])
 {
 	bool unlock;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 1) {
 		printf("--%s(-1)\n", __func__);
 		return CMD_RET_USAGE;
 	}
 
-	if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Unlocked = %d\n", unlock);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Can't determine device lock state.\n");
+	printf("Can't determine device lock state, err = %d\n",
+	       ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -354,11 +359,9 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	size_t bytes_read;
 	void *buffer;
 	char *endp;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 3)
 		return CMD_RET_USAGE;
@@ -372,15 +375,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!buffer)
 		return CMD_RET_FAILURE;
 
-	if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
-					   &bytes_read) == AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_persistent_value(avb_ops, name, bytes,
+					     buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Read %zu bytes, value = %s\n", bytes_read,
 		       (char *)buffer);
 		free(buffer);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read persistent value\n");
+	printf("Failed to read persistent value, err = %d\n", ret);
 
 	free(buffer);
 
@@ -392,11 +396,9 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	const char *name;
 	const char *value;
+	int ret;
 
-	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
-		return CMD_RET_FAILURE;
-	}
+	AVB_OPS_CHECK(avb_ops);
 
 	if (argc != 3)
 		return CMD_RET_USAGE;
@@ -404,14 +406,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	name = argv[1];
 	value = argv[2];
 
-	if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
-					    (const uint8_t *)value) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->write_persistent_value(avb_ops, name,
+					      strlen(value) + 1,
+					      (const uint8_t *)value);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Wrote %zu bytes\n", strlen(value) + 1);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to write persistent value\n");
+	printf("Failed to write persistent value `%s` = `%s`, err = %d\n",
+	       name, value, ret);
 
 	return CMD_RET_FAILURE;
 }
-- 
2.34.1

Re: [PATCH v1 4/7] cmd: avb: rework prints

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Introduce AVB_OPS_CHECK macro for checking AvbOps before using
> it to avoid code duplication.
> Simplify and add more context for prints where it's needed.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> ---
>
>  cmd/avb.c | 156 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 80 insertions(+), 76 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index ce8b63873f2..ae0012c0e79 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -11,6 +11,14 @@
>  #include <mmc.h>
>  
>  #define AVB_BOOTARGS	"avb_bootargs"
> +
> +#define AVB_OPS_CHECK(avb_ops) do { \
> +	if (!(avb_ops)) { \
> +		printf("AVB is not initialized, please run 'avb init <id>'\n"); \
> +		return CMD_RET_FAILURE; \
> +	} \
> +} while (false)
> +
checkpatch.pl --u-boot seems to complain about this:
WARNING: Macros with flow control statements should be avoided
#25: FILE: cmd/avb.c:15:

This seems documented in the kernel coding style as well.
https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl

I'd argue that in this case, it's not better or worse in terme of
readability but i'd prefer to stick to the rules in this case.

The error handling (using ret) and the better prints seem fine though.
Could we drop the AVB_OPS_CHECK macro for v2?

>  static struct AvbOps *avb_ops;
>  
>  int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> @@ -28,8 +36,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	avb_ops = avb_ops_alloc(mmc_dev);
>  	if (avb_ops)
>  		return CMD_RET_SUCCESS;
> +	else
> +		printf("Can't allocate AvbOps");
>  
> -	printf("Failed to initialize avb2\n");
> +	printf("Failed to initialize AVB\n");
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -41,11 +51,9 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes, bytes_read = 0;
>  	void *buffer;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> -		return CMD_RET_USAGE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 5)
>  		return CMD_RET_USAGE;
> @@ -55,14 +63,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	bytes = hextoul(argv[3], NULL);
>  	buffer = (void *)hextoul(argv[4], NULL);
>  
> -	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
> -					 buffer, &bytes_read) ==
> -					 AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_from_partition(avb_ops, part, offset,
> +					   bytes, buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Read %zu bytes\n", bytes_read);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read from partition\n");
> +	printf("Failed to read from partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -74,11 +83,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes, bytes_read = 0;
>  	char *buffer;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> -		return CMD_RET_USAGE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 4)
>  		return CMD_RET_USAGE;
> @@ -94,8 +101,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  	}
>  	memset(buffer, 0, bytes);
>  
> -	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
> -					 &bytes_read) == AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_from_partition(avb_ops, part, offset,
> +					   bytes, buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
>  		printf("Data: ");
>  		for (int i = 0; i < bytes_read; i++)
> @@ -107,7 +115,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read from partition\n");
> +	printf("Failed to read from partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	free(buffer);
>  	return CMD_RET_FAILURE;
> @@ -120,11 +129,9 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes;
>  	void *buffer;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 5)
>  		return CMD_RET_USAGE;
> @@ -134,13 +141,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	bytes = hextoul(argv[3], NULL);
>  	buffer = (void *)hextoul(argv[4], NULL);
>  
> -	if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->write_to_partition(avb_ops, part, offset,
> +					  bytes, buffer);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Wrote %zu bytes\n", bytes);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to write in partition\n");
> +	printf("Failed to write in partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -150,24 +159,23 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	size_t index;
>  	u64 rb_idx;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 2)
>  		return CMD_RET_USAGE;
>  
>  	index = (size_t)hextoul(argv[1], NULL);
>  
> -	if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Rollback index: %llx\n", rb_idx);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read rollback index\n");
> +	printf("Failed to read rollback index id = %zu, err = %d\n",
> +	       index, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -177,11 +185,9 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	size_t index;
>  	u64 rb_idx;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 3)
>  		return CMD_RET_USAGE;
> @@ -189,11 +195,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  	index = (size_t)hextoul(argv[1], NULL);
>  	rb_idx = hextoul(argv[2], NULL);
>  
> -	if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
> -	    AVB_IO_RESULT_OK)
> +	ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx);
> +	if (ret == AVB_IO_RESULT_OK)
>  		return CMD_RET_SUCCESS;
>  
> -	printf("Failed to write rollback index\n");
> +	printf("Failed to write rollback index id = %zu, err = %d\n",
> +	       index, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -203,25 +210,25 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
>  {
>  	const char *part;
>  	char buffer[UUID_STR_LEN + 1];
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 2)
>  		return CMD_RET_USAGE;
>  
>  	part = argv[1];
>  
> -	if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
> -						   UUID_STR_LEN + 1) ==
> -						   AVB_IO_RESULT_OK) {
> +	ret = avb_ops->get_unique_guid_for_partition(avb_ops, part,
> +						     buffer,
> +						     UUID_STR_LEN + 1);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("'%s' UUID: %s\n", part, buffer);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read UUID\n");
> +	printf("Failed to read partition '%s' UUID, err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -235,14 +242,12 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  	char *cmdline;
>  	char *extra_args;
>  	char *slot_suffix = "";
> +	int ret;
>  
>  	bool unlocked = false;
>  	int res = CMD_RET_FAILURE;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc < 1 || argc > 2)
>  		return CMD_RET_USAGE;
> @@ -253,9 +258,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  	printf("## Android Verified Boot 2.0 version %s\n",
>  	       avb_version_string());
>  
> -	if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
> -	    AVB_IO_RESULT_OK) {
> -		printf("Can't determine device lock state.\n");
> +	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
> +	if (ret != AVB_IO_RESULT_OK) {
> +		printf("Can't determine device lock state, err = %d\n",
> +		       ret);
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -302,10 +308,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  		printf("Corrupted dm-verity metadata detected\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> -		printf("Unsupported version avbtool was used\n");
> +		printf("Unsupported version of avbtool was used\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> -		printf("Checking rollback index failed\n");
> +		printf("Rollback index check failed\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
>  		printf("Public key was rejected\n");
> @@ -324,24 +330,23 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
>  		       int argc, char *const argv[])
>  {
>  	bool unlock;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 1) {
>  		printf("--%s(-1)\n", __func__);
>  		return CMD_RET_USAGE;
>  	}
>  
> -	if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Unlocked = %d\n", unlock);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Can't determine device lock state.\n");
> +	printf("Can't determine device lock state, err = %d\n",
> +	       ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -354,11 +359,9 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	size_t bytes_read;
>  	void *buffer;
>  	char *endp;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 3)
>  		return CMD_RET_USAGE;
> @@ -372,15 +375,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	if (!buffer)
>  		return CMD_RET_FAILURE;
>  
> -	if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
> -					   &bytes_read) == AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_persistent_value(avb_ops, name, bytes,
> +					     buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Read %zu bytes, value = %s\n", bytes_read,
>  		       (char *)buffer);
>  		free(buffer);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read persistent value\n");
> +	printf("Failed to read persistent value, err = %d\n", ret);
>  
>  	free(buffer);
>  
> @@ -392,11 +396,9 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	const char *name;
>  	const char *value;
> +	int ret;
>  
> -	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	AVB_OPS_CHECK(avb_ops);
>  
>  	if (argc != 3)
>  		return CMD_RET_USAGE;
> @@ -404,14 +406,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	name = argv[1];
>  	value = argv[2];
>  
> -	if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
> -					    (const uint8_t *)value) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->write_persistent_value(avb_ops, name,
> +					      strlen(value) + 1,
> +					      (const uint8_t *)value);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Wrote %zu bytes\n", strlen(value) + 1);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to write persistent value\n");
> +	printf("Failed to write persistent value `%s` = `%s`, err = %d\n",
> +	       name, value, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> -- 
> 2.34.1

Re: [PATCH v1 4/7] cmd: avb: rework prints

[Igor Opaniuk]

Hi Mattijs,

On Thu, Feb 8, 2024 at 3:00 PM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Igor,
>
> Thank you for the patch.
>
> On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
> >
> > Introduce AVB_OPS_CHECK macro for checking AvbOps before using
> > it to avoid code duplication.
> > Simplify and add more context for prints where it's needed.
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> > ---
> >
> >  cmd/avb.c | 156 ++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 80 insertions(+), 76 deletions(-)
> >
> > diff --git a/cmd/avb.c b/cmd/avb.c
> > index ce8b63873f2..ae0012c0e79 100644
> > --- a/cmd/avb.c
> > +++ b/cmd/avb.c
> > @@ -11,6 +11,14 @@
> >  #include <mmc.h>
> >
> >  #define AVB_BOOTARGS "avb_bootargs"
> > +
> > +#define AVB_OPS_CHECK(avb_ops) do { \
> > +     if (!(avb_ops)) { \
> > +             printf("AVB is not initialized, please run 'avb init <id>'\n"); \
> > +             return CMD_RET_FAILURE; \
> > +     } \
> > +} while (false)
> > +
> checkpatch.pl --u-boot seems to complain about this:
> WARNING: Macros with flow control statements should be avoided
> #25: FILE: cmd/avb.c:15:
>
> This seems documented in the kernel coding style as well.
> https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
>
> I'd argue that in this case, it's not better or worse in terme of
> readability but i'd prefer to stick to the rules in this case.
>
> The error handling (using ret) and the better prints seem fine though.
> Could we drop the AVB_OPS_CHECK macro for v2?
Yeah, I was just trying to find a tradeoff between readability/code duplication
and coding style.

Will drop it in v2. Thanks!

>
> >  static struct AvbOps *avb_ops;
> >
> >  int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > @@ -28,8 +36,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >       avb_ops = avb_ops_alloc(mmc_dev);
> >       if (avb_ops)
> >               return CMD_RET_SUCCESS;
> > +     else
> > +             printf("Can't allocate AvbOps");
> >
> > -     printf("Failed to initialize avb2\n");
> > +     printf("Failed to initialize AVB\n");
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -41,11 +51,9 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
> >       s64 offset;
> >       size_t bytes, bytes_read = 0;
> >       void *buffer;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> > -             return CMD_RET_USAGE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 5)
> >               return CMD_RET_USAGE;
> > @@ -55,14 +63,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
> >       bytes = hextoul(argv[3], NULL);
> >       buffer = (void *)hextoul(argv[4], NULL);
> >
> > -     if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
> > -                                      buffer, &bytes_read) ==
> > -                                      AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->read_from_partition(avb_ops, part, offset,
> > +                                        bytes, buffer, &bytes_read);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Read %zu bytes\n", bytes_read);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to read from partition\n");
> > +     printf("Failed to read from partition '%s', err = %d\n",
> > +            part, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -74,11 +83,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
> >       s64 offset;
> >       size_t bytes, bytes_read = 0;
> >       char *buffer;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> > -             return CMD_RET_USAGE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 4)
> >               return CMD_RET_USAGE;
> > @@ -94,8 +101,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
> >       }
> >       memset(buffer, 0, bytes);
> >
> > -     if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
> > -                                      &bytes_read) == AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->read_from_partition(avb_ops, part, offset,
> > +                                        bytes, buffer, &bytes_read);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
> >               printf("Data: ");
> >               for (int i = 0; i < bytes_read; i++)
> > @@ -107,7 +115,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to read from partition\n");
> > +     printf("Failed to read from partition '%s', err = %d\n",
> > +            part, ret);
> >
> >       free(buffer);
> >       return CMD_RET_FAILURE;
> > @@ -120,11 +129,9 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
> >       s64 offset;
> >       size_t bytes;
> >       void *buffer;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 5)
> >               return CMD_RET_USAGE;
> > @@ -134,13 +141,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
> >       bytes = hextoul(argv[3], NULL);
> >       buffer = (void *)hextoul(argv[4], NULL);
> >
> > -     if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
> > -         AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->write_to_partition(avb_ops, part, offset,
> > +                                       bytes, buffer);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Wrote %zu bytes\n", bytes);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to write in partition\n");
> > +     printf("Failed to write in partition '%s', err = %d\n",
> > +            part, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -150,24 +159,23 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
> >  {
> >       size_t index;
> >       u64 rb_idx;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 2)
> >               return CMD_RET_USAGE;
> >
> >       index = (size_t)hextoul(argv[1], NULL);
> >
> > -     if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
> > -         AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Rollback index: %llx\n", rb_idx);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to read rollback index\n");
> > +     printf("Failed to read rollback index id = %zu, err = %d\n",
> > +            index, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -177,11 +185,9 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
> >  {
> >       size_t index;
> >       u64 rb_idx;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 3)
> >               return CMD_RET_USAGE;
> > @@ -189,11 +195,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
> >       index = (size_t)hextoul(argv[1], NULL);
> >       rb_idx = hextoul(argv[2], NULL);
> >
> > -     if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
> > -         AVB_IO_RESULT_OK)
> > +     ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx);
> > +     if (ret == AVB_IO_RESULT_OK)
> >               return CMD_RET_SUCCESS;
> >
> > -     printf("Failed to write rollback index\n");
> > +     printf("Failed to write rollback index id = %zu, err = %d\n",
> > +            index, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -203,25 +210,25 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
> >  {
> >       const char *part;
> >       char buffer[UUID_STR_LEN + 1];
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 2)
> >               return CMD_RET_USAGE;
> >
> >       part = argv[1];
> >
> > -     if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
> > -                                                UUID_STR_LEN + 1) ==
> > -                                                AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->get_unique_guid_for_partition(avb_ops, part,
> > +                                                  buffer,
> > +                                                  UUID_STR_LEN + 1);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("'%s' UUID: %s\n", part, buffer);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to read UUID\n");
> > +     printf("Failed to read partition '%s' UUID, err = %d\n",
> > +            part, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -235,14 +242,12 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
> >       char *cmdline;
> >       char *extra_args;
> >       char *slot_suffix = "";
> > +     int ret;
> >
> >       bool unlocked = false;
> >       int res = CMD_RET_FAILURE;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc < 1 || argc > 2)
> >               return CMD_RET_USAGE;
> > @@ -253,9 +258,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
> >       printf("## Android Verified Boot 2.0 version %s\n",
> >              avb_version_string());
> >
> > -     if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
> > -         AVB_IO_RESULT_OK) {
> > -             printf("Can't determine device lock state.\n");
> > +     ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
> > +     if (ret != AVB_IO_RESULT_OK) {
> > +             printf("Can't determine device lock state, err = %d\n",
> > +                    ret);
> >               return CMD_RET_FAILURE;
> >       }
> >
> > @@ -302,10 +308,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
> >               printf("Corrupted dm-verity metadata detected\n");
> >               break;
> >       case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> > -             printf("Unsupported version avbtool was used\n");
> > +             printf("Unsupported version of avbtool was used\n");
> >               break;
> >       case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> > -             printf("Checking rollback index failed\n");
> > +             printf("Rollback index check failed\n");
> >               break;
> >       case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
> >               printf("Public key was rejected\n");
> > @@ -324,24 +330,23 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
> >                      int argc, char *const argv[])
> >  {
> >       bool unlock;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 1) {
> >               printf("--%s(-1)\n", __func__);
> >               return CMD_RET_USAGE;
> >       }
> >
> > -     if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
> > -         AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Unlocked = %d\n", unlock);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Can't determine device lock state.\n");
> > +     printf("Can't determine device lock state, err = %d\n",
> > +            ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > @@ -354,11 +359,9 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
> >       size_t bytes_read;
> >       void *buffer;
> >       char *endp;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 3)
> >               return CMD_RET_USAGE;
> > @@ -372,15 +375,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
> >       if (!buffer)
> >               return CMD_RET_FAILURE;
> >
> > -     if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
> > -                                        &bytes_read) == AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->read_persistent_value(avb_ops, name, bytes,
> > +                                          buffer, &bytes_read);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Read %zu bytes, value = %s\n", bytes_read,
> >                      (char *)buffer);
> >               free(buffer);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to read persistent value\n");
> > +     printf("Failed to read persistent value, err = %d\n", ret);
> >
> >       free(buffer);
> >
> > @@ -392,11 +396,9 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
> >  {
> >       const char *name;
> >       const char *value;
> > +     int ret;
> >
> > -     if (!avb_ops) {
> > -             printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> > -             return CMD_RET_FAILURE;
> > -     }
> > +     AVB_OPS_CHECK(avb_ops);
> >
> >       if (argc != 3)
> >               return CMD_RET_USAGE;
> > @@ -404,14 +406,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
> >       name = argv[1];
> >       value = argv[2];
> >
> > -     if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
> > -                                         (const uint8_t *)value) ==
> > -         AVB_IO_RESULT_OK) {
> > +     ret = avb_ops->write_persistent_value(avb_ops, name,
> > +                                           strlen(value) + 1,
> > +                                           (const uint8_t *)value);
> > +     if (ret == AVB_IO_RESULT_OK) {
> >               printf("Wrote %zu bytes\n", strlen(value) + 1);
> >               return CMD_RET_SUCCESS;
> >       }
> >
> > -     printf("Failed to write persistent value\n");
> > +     printf("Failed to write persistent value `%s` = `%s`, err = %d\n",
> > +            name, value, ret);
> >
> >       return CMD_RET_FAILURE;
> >  }
> > --
> > 2.34.1



-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk@foundries.io
W: www.foundries.io

[PATCH v1 5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Introduce str_avb_io_error() and str_avb_slot_error() functions,
that provide a pointer to AVB runtime error message.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 common/avb_verify.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/avb_verify.h |  3 ++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index ed58239cf8a..cff9117d92f 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -119,6 +119,55 @@ static const unsigned char avb_root_pub[1032] = {
 	0xd8, 0x7e,
 };
 
+const char *str_avb_io_error(AvbIOResult res)
+{
+	switch (res) {
+	case AVB_IO_RESULT_OK:
+		return "Requested operation was successful";
+	case AVB_IO_RESULT_ERROR_IO:
+		return "Underlying hardware encountered an I/O error";
+	case AVB_IO_RESULT_ERROR_OOM:
+		return "Unable to allocate memory";
+	case AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION:
+		return "Requested partition does not exist";
+	case AVB_IO_RESULT_ERROR_RANGE_OUTSIDE_PARTITION:
+		return "Bytes requested is outside the range of partition";
+	case AVB_IO_RESULT_ERROR_NO_SUCH_VALUE:
+		return "Named persistent value does not exist";
+	case AVB_IO_RESULT_ERROR_INVALID_VALUE_SIZE:
+		return "Named persistent value size is not supported";
+	case AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE:
+		return "Buffer is too small for the requested operation";
+	default:
+		return "Unknown AVB error";
+	}
+}
+
+const char *str_avb_slot_error(AvbSlotVerifyResult res)
+{
+	switch (res) {
+	case AVB_SLOT_VERIFY_RESULT_OK:
+		return "Verification passed successfully";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
+		return "Allocation of memory failed";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
+		return "I/O error occurred while trying to load data";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
+		return "Digest didn't match or signature checks failed";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
+		return "Rollback index is less than its stored value";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
+		return "Public keys are not accepted";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
+		return "Metadata is invalid or inconsistent";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
+		return "Metadata requires a newer version of libavb";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT:
+		return "Invalid arguments are used";
+	default:
+		return "Unknown AVB slot verification error";
+	}
+}
 /**
  * ============================================================================
  * Boot states support (GREEN, YELLOW, ORANGE, RED) and dm_verity
diff --git a/include/avb_verify.h b/include/avb_verify.h
index 2fb850044d9..5d998b5a302 100644
--- a/include/avb_verify.h
+++ b/include/avb_verify.h
@@ -52,7 +52,8 @@ char *avb_set_enforce_verity(const char *cmdline);
 char *avb_set_ignore_corruption(const char *cmdline);
 
 char *append_cmd_line(char *cmdline_orig, char *cmdline_new);
-
+const char *str_avb_io_error(AvbIOResult res);
+const char *str_avb_slot_error(AvbSlotVerifyResult res);
 /**
  * ============================================================================
  * I/O helper inline functions
-- 
2.34.1

Re: [PATCH v1 5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Introduce str_avb_io_error() and str_avb_slot_error() functions,
> that provide a pointer to AVB runtime error message.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
>  common/avb_verify.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  include/avb_verify.h |  3 ++-
>  2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index ed58239cf8a..cff9117d92f 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -119,6 +119,55 @@ static const unsigned char avb_root_pub[1032] = {
>  	0xd8, 0x7e,
>  };
>  
> +const char *str_avb_io_error(AvbIOResult res)
> +{
> +	switch (res) {
> +	case AVB_IO_RESULT_OK:
> +		return "Requested operation was successful";
> +	case AVB_IO_RESULT_ERROR_IO:
> +		return "Underlying hardware encountered an I/O error";
> +	case AVB_IO_RESULT_ERROR_OOM:
> +		return "Unable to allocate memory";
> +	case AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION:
> +		return "Requested partition does not exist";
> +	case AVB_IO_RESULT_ERROR_RANGE_OUTSIDE_PARTITION:
> +		return "Bytes requested is outside the range of partition";
> +	case AVB_IO_RESULT_ERROR_NO_SUCH_VALUE:
> +		return "Named persistent value does not exist";
> +	case AVB_IO_RESULT_ERROR_INVALID_VALUE_SIZE:
> +		return "Named persistent value size is not supported";
> +	case AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE:
> +		return "Buffer is too small for the requested operation";
> +	default:
> +		return "Unknown AVB error";
> +	}
> +}
> +
> +const char *str_avb_slot_error(AvbSlotVerifyResult res)
> +{
> +	switch (res) {
> +	case AVB_SLOT_VERIFY_RESULT_OK:
> +		return "Verification passed successfully";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
> +		return "Allocation of memory failed";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
> +		return "I/O error occurred while trying to load data";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
> +		return "Digest didn't match or signature checks failed";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> +		return "Rollback index is less than its stored value";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
> +		return "Public keys are not accepted";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
> +		return "Metadata is invalid or inconsistent";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> +		return "Metadata requires a newer version of libavb";
> +	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT:
> +		return "Invalid arguments are used";
> +	default:
> +		return "Unknown AVB slot verification error";
> +	}
> +}
>  /**
>   * ============================================================================
>   * Boot states support (GREEN, YELLOW, ORANGE, RED) and dm_verity
> diff --git a/include/avb_verify.h b/include/avb_verify.h
> index 2fb850044d9..5d998b5a302 100644
> --- a/include/avb_verify.h
> +++ b/include/avb_verify.h
> @@ -52,7 +52,8 @@ char *avb_set_enforce_verity(const char *cmdline);
>  char *avb_set_ignore_corruption(const char *cmdline);
>  
>  char *append_cmd_line(char *cmdline_orig, char *cmdline_new);
> -
> +const char *str_avb_io_error(AvbIOResult res);
> +const char *str_avb_slot_error(AvbSlotVerifyResult res);
>  /**
>   * ============================================================================
>   * I/O helper inline functions
> -- 
> 2.34.1

[PATCH v1 6/7] cmd: avb: rework do_avb_verify_part

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Use existing str_avb_slot_error() function for obtaining
verification fail reason details.
Take into account device lock state for setting correct
androidboot.verifiedbootstate kernel cmdline parameter.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 cmd/avb.c | 50 +++++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index ae0012c0e79..e5fc202121f 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -239,6 +239,7 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	const char * const requested_partitions[] = {"boot", NULL};
 	AvbSlotVerifyResult slot_result;
 	AvbSlotVerifyData *out_data;
+	enum avb_boot_state boot_state;
 	char *cmdline;
 	char *extra_args;
 	char *slot_suffix = "";
@@ -273,18 +274,23 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 				AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE,
 				&out_data);
 
-	switch (slot_result) {
-	case AVB_SLOT_VERIFY_RESULT_OK:
-		/* Until we don't have support of changing unlock states, we
-		 * assume that we are by default in locked state.
-		 * So in this case we can boot only when verification is
-		 * successful; we also supply in cmdline GREEN boot state
-		 */
+	/*
+	 * LOCKED devices with custom root of trust setup is not supported (YELLOW)
+	 */
+	if (slot_result == AVB_SLOT_VERIFY_RESULT_OK) {
 		printf("Verification passed successfully\n");
 
-		/* export additional bootargs to AVB_BOOTARGS env var */
+		/*
+		 * ORANGE state indicates that device may be freely modified.
+		 * Device integrity is left to the user to verify out-of-band.
+		 */
+		if (unlocked)
+			boot_state = AVB_ORANGE;
+		else
+			boot_state = AVB_GREEN;
 
-		extra_args = avb_set_state(avb_ops, AVB_GREEN);
+		/* export boot state to AVB_BOOTARGS env var */
+		extra_args = avb_set_state(avb_ops, boot_state);
 		if (extra_args)
 			cmdline = append_cmd_line(out_data->cmdline,
 						  extra_args);
@@ -294,30 +300,8 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 		env_set(AVB_BOOTARGS, cmdline);
 
 		res = CMD_RET_SUCCESS;
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
-		printf("Verification failed\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
-		printf("I/O error occurred during verification\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
-		printf("OOM error occurred during verification\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
-		printf("Corrupted dm-verity metadata detected\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
-		printf("Unsupported version of avbtool was used\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
-		printf("Rollback index check failed\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
-		printf("Public key was rejected\n");
-		break;
-	default:
-		printf("Unknown error occurred\n");
+	} else {
+		printf("Verification failed, reason: %s\n", str_avb_slot_error(slot_result));
 	}
 
 	if (out_data)
-- 
2.34.1

Re: [PATCH v1 6/7] cmd: avb: rework do_avb_verify_part

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Use existing str_avb_slot_error() function for obtaining
> verification fail reason details.
> Take into account device lock state for setting correct
> androidboot.verifiedbootstate kernel cmdline parameter.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Nitpick: the error handling migration could have been part of PATCH 5/7
but it's fine to keep as is.

> ---
>
>  cmd/avb.c | 50 +++++++++++++++++---------------------------------
>  1 file changed, 17 insertions(+), 33 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index ae0012c0e79..e5fc202121f 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -239,6 +239,7 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  	const char * const requested_partitions[] = {"boot", NULL};
>  	AvbSlotVerifyResult slot_result;
>  	AvbSlotVerifyData *out_data;
> +	enum avb_boot_state boot_state;
>  	char *cmdline;
>  	char *extra_args;
>  	char *slot_suffix = "";
> @@ -273,18 +274,23 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  				AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE,
>  				&out_data);
>  
> -	switch (slot_result) {
> -	case AVB_SLOT_VERIFY_RESULT_OK:
> -		/* Until we don't have support of changing unlock states, we
> -		 * assume that we are by default in locked state.
> -		 * So in this case we can boot only when verification is
> -		 * successful; we also supply in cmdline GREEN boot state
> -		 */
> +	/*
> +	 * LOCKED devices with custom root of trust setup is not supported (YELLOW)
> +	 */
> +	if (slot_result == AVB_SLOT_VERIFY_RESULT_OK) {
>  		printf("Verification passed successfully\n");
>  
> -		/* export additional bootargs to AVB_BOOTARGS env var */
> +		/*
> +		 * ORANGE state indicates that device may be freely modified.
> +		 * Device integrity is left to the user to verify out-of-band.
> +		 */
> +		if (unlocked)
> +			boot_state = AVB_ORANGE;
> +		else
> +			boot_state = AVB_GREEN;
>  
> -		extra_args = avb_set_state(avb_ops, AVB_GREEN);
> +		/* export boot state to AVB_BOOTARGS env var */
> +		extra_args = avb_set_state(avb_ops, boot_state);
>  		if (extra_args)
>  			cmdline = append_cmd_line(out_data->cmdline,
>  						  extra_args);
> @@ -294,30 +300,8 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  		env_set(AVB_BOOTARGS, cmdline);
>  
>  		res = CMD_RET_SUCCESS;
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
> -		printf("Verification failed\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
> -		printf("I/O error occurred during verification\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
> -		printf("OOM error occurred during verification\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
> -		printf("Corrupted dm-verity metadata detected\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> -		printf("Unsupported version of avbtool was used\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> -		printf("Rollback index check failed\n");
> -		break;
> -	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
> -		printf("Public key was rejected\n");
> -		break;
> -	default:
> -		printf("Unknown error occurred\n");
> +	} else {
> +		printf("Verification failed, reason: %s\n", str_avb_slot_error(slot_result));
>  	}
>  
>  	if (out_data)
> -- 
> 2.34.1

[PATCH v1 7/7] doc: android: avb: add slot_suffix param details

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Add info about slot_suffix param for avb verify.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

 doc/android/avb2.rst | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst
index a072119574f..c0b2bedb831 100644
--- a/doc/android/avb2.rst
+++ b/doc/android/avb2.rst
@@ -39,15 +39,17 @@ Provides CLI interface to invoke AVB 2.0 verification + misc. commands for
 different testing purposes::
 
     avb init <dev> - initialize avb 2.0 for <dev>
-    avb verify - run verification process using hash data from vbmeta structure
+    avb verify [slot_suffix] - run verification process using hash data
+        from vbmeta structure. Provide [slot_suffix] if vbmeta partition
+        is slotted
     avb read_rb <num> - read rollback index at location <num>
     avb write_rb <num> <rb> - write rollback index <rb> to <num>
     avb is_unlocked - returns unlock status of the device
     avb get_uuid <partname> - read and print uuid of partition <partname>
     avb read_part <partname> <offset> <num> <addr> - read <num> bytes from
-    partition <partname> to buffer <addr>
+        partition <partname> to buffer <addr>
     avb write_part <partname> <offset> <num> <addr> - write <num> bytes to
-    <partname> by <offset> using data from <addr>
+        <partname> by <offset> using data from <addr>
 
 Partitions tampering (example)
 ------------------------------
-- 
2.34.1

Re: [PATCH v1 7/7] doc: android: avb: add slot_suffix param details

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Add info about slot_suffix param for avb verify.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> ---
>
>  doc/android/avb2.rst | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst
> index a072119574f..c0b2bedb831 100644
> --- a/doc/android/avb2.rst
> +++ b/doc/android/avb2.rst
> @@ -39,15 +39,17 @@ Provides CLI interface to invoke AVB 2.0 verification + misc. commands for
>  different testing purposes::
>  
>      avb init <dev> - initialize avb 2.0 for <dev>
> -    avb verify - run verification process using hash data from vbmeta structure
> +    avb verify [slot_suffix] - run verification process using hash data
> +        from vbmeta structure. Provide [slot_suffix] if vbmeta partition
> +        is slotted

Any particular reason for this to not be exactly the wording as in cmd/avb.c?

	"avb verify [slot_suffix] - run verification process using hash data\n"
	"    from vbmeta structure\n"
	"    [slot_suffix] - _a, _b, etc (if vbmeta partition is slotted)\n"

It looks good, but I think it would be better if both are the same for
consistency, since both texts are user facing.

>      avb read_rb <num> - read rollback index at location <num>
>      avb write_rb <num> <rb> - write rollback index <rb> to <num>
>      avb is_unlocked - returns unlock status of the device
>      avb get_uuid <partname> - read and print uuid of partition <partname>
>      avb read_part <partname> <offset> <num> <addr> - read <num> bytes from
> -    partition <partname> to buffer <addr>
> +        partition <partname> to buffer <addr>
>      avb write_part <partname> <offset> <num> <addr> - write <num> bytes to
> -    <partname> by <offset> using data from <addr>
> +        <partname> by <offset> using data from <addr>
>  
>  Partitions tampering (example)
>  ------------------------------
> -- 
> 2.34.1

Re: [PATCH v1 7/7] doc: android: avb: add slot_suffix param details

[Igor Opaniuk]

Hi Mattijs,

On Thu, Feb 8, 2024 at 3:12 PM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Igor,
>
> Thank you for the patch.
>
> On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
> >
> > Add info about slot_suffix param for avb verify.
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> > ---
> >
> >  doc/android/avb2.rst | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst
> > index a072119574f..c0b2bedb831 100644
> > --- a/doc/android/avb2.rst
> > +++ b/doc/android/avb2.rst
> > @@ -39,15 +39,17 @@ Provides CLI interface to invoke AVB 2.0 verification + misc. commands for
> >  different testing purposes::
> >
> >      avb init <dev> - initialize avb 2.0 for <dev>
> > -    avb verify - run verification process using hash data from vbmeta structure
> > +    avb verify [slot_suffix] - run verification process using hash data
> > +        from vbmeta structure. Provide [slot_suffix] if vbmeta partition
> > +        is slotted
>
> Any particular reason for this to not be exactly the wording as in cmd/avb.c?
>
>         "avb verify [slot_suffix] - run verification process using hash data\n"
>         "    from vbmeta structure\n"
>         "    [slot_suffix] - _a, _b, etc (if vbmeta partition is slotted)\n"
>
> It looks good, but I think it would be better if both are the same for
> consistency, since both texts are user facing.
Will fix it in v2, thanks!

>
> >      avb read_rb <num> - read rollback index at location <num>
> >      avb write_rb <num> <rb> - write rollback index <rb> to <num>
> >      avb is_unlocked - returns unlock status of the device
> >      avb get_uuid <partname> - read and print uuid of partition <partname>
> >      avb read_part <partname> <offset> <num> <addr> - read <num> bytes from
> > -    partition <partname> to buffer <addr>
> > +        partition <partname> to buffer <addr>
> >      avb write_part <partname> <offset> <num> <addr> - write <num> bytes to
> > -    <partname> by <offset> using data from <addr>
> > +        <partname> by <offset> using data from <addr>
> >
> >  Partitions tampering (example)
> >  ------------------------------
> > --
> > 2.34.1



-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk@foundries.io
W: www.foundries.io