[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Sam Protsenko]

The requested_partitions[] array should contain only boot partitions.
Usually it's only 'boot' partition, as can be seen in [1]. Also, seems
like the requested_partitions[] are only used when there is no 'vbmeta'
partition [2], which is not a regular use-case.

Make requested_partitions[] contain only 'boot' partition as it was
supposed to be, and also make that array to be a local in
do_avb_verify_part() function, as nobody else needs that.

[1] https://android.googlesource.com/platform/external/avb/+/master/test/avb_slot_verify_unittest.cc#108
[2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_slot_verify.c#1461

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 cmd/avb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index d1942d6605..8f2bb85fce 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -14,11 +14,6 @@
 #define AVB_BOOTARGS	"avb_bootargs"
 static struct AvbOps *avb_ops;
 
-static const char * const requested_partitions[] = {"boot",
-					     "system",
-					     "vendor",
-					     NULL};
-
 int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	unsigned long mmc_dev;
@@ -231,6 +226,7 @@ int do_avb_get_uuid(cmd_tbl_t *cmdtp, int flag,
 int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
 		       int argc, char *const argv[])
 {
+	const char * const requested_partitions[] = {"boot", NULL};
 	AvbSlotVerifyResult slot_result;
 	AvbSlotVerifyData *out_data;
 	char *cmdline;
-- 
2.20.1

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Igor Opaniuk]

Hi Sam,

On Fri, Aug 9, 2019 at 7:16 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> The requested_partitions[] array should contain only boot partitions.
> Usually it's only 'boot' partition, as can be seen in [1]. Also, seems
> like the requested_partitions[] are only used when there is no 'vbmeta'
> partition [2], which is not a regular use-case.
>
> Make requested_partitions[] contain only 'boot' partition as it was
> supposed to be, and also make that array to be a local in
> do_avb_verify_part() function, as nobody else needs that.
>
> [1] https://android.googlesource.com/platform/external/avb/+/master/test/avb_slot_verify_unittest.cc#108
> [2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_slot_verify.c#1461
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  cmd/avb.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index d1942d6605..8f2bb85fce 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -14,11 +14,6 @@
>  #define AVB_BOOTARGS   "avb_bootargs"
>  static struct AvbOps *avb_ops;
>
> -static const char * const requested_partitions[] = {"boot",
> -                                            "system",
> -                                            "vendor",
> -                                            NULL};
> -
>  int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>         unsigned long mmc_dev;
> @@ -231,6 +226,7 @@ int do_avb_get_uuid(cmd_tbl_t *cmdtp, int flag,
>  int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
>                        int argc, char *const argv[])
>  {
> +       const char * const requested_partitions[] = {"boot", NULL};
>         AvbSlotVerifyResult slot_result;
>         AvbSlotVerifyData *out_data;
>         char *cmdline;
> --
> 2.20.1
>
Thanks for the patch.

Current snapshot of libavb in U-boot is a bit out-dated (~2 years) and
before introducing patches that can leverage new features from the mainline
libavb(taking into account that you're referring to Jul 30, 2019 patch
36d41d9223 ("Add AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag"))
it makes sense to update libavb in U-boot first:

The best approach I see here (just to summarize what we've already discussed):
1. Update libavb to the latest stable version
2. Check if `avb verify` still behaves as expected on non-AB setups.
3. Introduce support of AB slots and different fixes like this one

Thanks

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

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Eugeniu Rosca]

Hi all,

On Mon, Aug 12, 2019 at 02:13:55PM +0300, Igor Opaniuk wrote:
[..]
> Current snapshot of libavb in U-boot is a bit out-dated (~2 years) and
> before introducing patches that can leverage new features from the mainline
> libavb(taking into account that you're referring to Jul 30, 2019 patch
> 36d41d9223 ("Add AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag"))
> it makes sense to update libavb in U-boot first:
> 
> The best approach I see here (just to summarize what we've already discussed):
> 1. Update libavb to the latest stable version

FWIW, this sounds good. jFYI, U-Boot seems to contain the
android-o-mr1-iot-preview-8 version of libavb (heuristics applied since
commit [1] didn't include any information about the version).

FWIW, in case of an alignment to AOSP, the list of libavb commits to
be backported is shown in [2]. The diff stats is visible in [3].

FWIW w.r.t. integration strategy, we better use
'git cherry-pick --strategy=subtree -Xsubtree=lib/' instead of manual
copying the libavb contents from
https://android.googlesource.com/platform/external/avb/, since the
latter would overwrite local U-Boot fixes like commit [4].

[1] https://gitlab.denx.de/u-boot/u-boot/commit/d8f9d2af96b38f
    ("avb2.0: add Android Verified Boot 2.0 library")

[2] avb (master) git log --oneline --no-merges --no-decorate \
    android-o-mr1-iot-preview-8..5fbb42a189aa -- libavb
36d41d922380 Add AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag.
3deffc556cfa Fix the identification of vbmeta partition
2ea684dbd612 Adds avb_strncmp helper
31f5f91352e6 Libavb: Check rollback index location against 0.
0a4e345d73f9 Fix a bug that may cause inifinite loop.
d91e04e2c163 Fix a bug that would cause OoB read.
18b535905bab Fix a stack-use-after-scope bug.
5786fa2f2cfa Fix a memory leak bug.
a6c9ad41f779 Fix a bug that would cause OoB memory read.
9ba3b6613b4e Fix AvbSlotVerifyData->cmdline might be NULL
5abd6bc25789 Allow system partition to be absent
8127dae6e4f5 Only automatically initialize new persistent digest values when locked
4f137c38f838 libavb: Use size_t instead of uint32_t for lengths in avb_sha*() functions.
818cf5674077 libavb: Only query partition GUIDs when the cmdline needs them.
49936b4c0109 libavb: Support vbmeta blobs in beginning of partition.
6f4de3181429 libavb: Add ERROR_MODE_MANAGED_RESTART_AND_EIO for managing dm-verity state.
2367b46287bd Implement initialization of new persistent digest values

[3] avb (master) git diff --shortstat \
    android-o-mr1-iot-preview-8..5fbb42a189aa -- libavb
 12 files changed, 704 insertions(+), 208 deletions(-)

[4] https://gitlab.denx.de/u-boot/u-boot/commit/ecc6f6bea6a2
    ("libavb: Handle wrong hashtree_error_mode in avb_append_options()")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Eugeniu Rosca]

Hi Sam,

On Fri, Aug 09, 2019 at 07:16:03PM +0300, Sam Protsenko wrote:
> The requested_partitions[] array should contain only boot partitions.
> Usually it's only 'boot' partition, as can be seen in [1]. Also, seems
> like the requested_partitions[] are only used when there is no 'vbmeta'
> partition [2], which is not a regular use-case.
> 
> Make requested_partitions[] contain only 'boot' partition as it was
> supposed to be, and also make that array to be a local in
> do_avb_verify_part() function, as nobody else needs that.
> 
> [1] https://android.googlesource.com/platform/external/avb/+/master/test/avb_slot_verify_unittest.cc#108
> [2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_slot_verify.c#1461

The patches are much appreciated. Could we agree to avoid volatile
references in the links, since those will point out to wrong lines after
a couple of weeks? I think it's safer to either use the latest available
commit id or tag, e.g.:

[1] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/test/avb_slot_verify_unittest.cc#108
[2] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/libavb/avb_slot_verify.c#1461

Thank you.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Sam Protsenko]

Hi Eugeniu,

On Mon, Aug 12, 2019 at 4:57 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi all,
>
> On Mon, Aug 12, 2019 at 02:13:55PM +0300, Igor Opaniuk wrote:
> [..]
> > Current snapshot of libavb in U-boot is a bit out-dated (~2 years) and
> > before introducing patches that can leverage new features from the mainline
> > libavb(taking into account that you're referring to Jul 30, 2019 patch
> > 36d41d9223 ("Add AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag"))
> > it makes sense to update libavb in U-boot first:
> >
> > The best approach I see here (just to summarize what we've already discussed):
> > 1. Update libavb to the latest stable version
>
> FWIW, this sounds good. jFYI, U-Boot seems to contain the
> android-o-mr1-iot-preview-8 version of libavb (heuristics applied since
> commit [1] didn't include any information about the version).
>
> FWIW, in case of an alignment to AOSP, the list of libavb commits to
> be backported is shown in [2]. The diff stats is visible in [3].
>
> FWIW w.r.t. integration strategy, we better use
> 'git cherry-pick --strategy=subtree -Xsubtree=lib/' instead of manual
> copying the libavb contents from
> https://android.googlesource.com/platform/external/avb/, since the
> latter would overwrite local U-Boot fixes like commit [4].
>

Wow, that's some real jedi git skills here :) Thanks, Eugeniu, will
use that when porting most recent libavb to U-Boot.

> [1] https://gitlab.denx.de/u-boot/u-boot/commit/d8f9d2af96b38f
>     ("avb2.0: add Android Verified Boot 2.0 library")
>
> [2] avb (master) git log --oneline --no-merges --no-decorate \
>     android-o-mr1-iot-preview-8..5fbb42a189aa -- libavb
> 36d41d922380 Add AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag.
> 3deffc556cfa Fix the identification of vbmeta partition
> 2ea684dbd612 Adds avb_strncmp helper
> 31f5f91352e6 Libavb: Check rollback index location against 0.
> 0a4e345d73f9 Fix a bug that may cause inifinite loop.
> d91e04e2c163 Fix a bug that would cause OoB read.
> 18b535905bab Fix a stack-use-after-scope bug.
> 5786fa2f2cfa Fix a memory leak bug.
> a6c9ad41f779 Fix a bug that would cause OoB memory read.
> 9ba3b6613b4e Fix AvbSlotVerifyData->cmdline might be NULL
> 5abd6bc25789 Allow system partition to be absent
> 8127dae6e4f5 Only automatically initialize new persistent digest values when locked
> 4f137c38f838 libavb: Use size_t instead of uint32_t for lengths in avb_sha*() functions.
> 818cf5674077 libavb: Only query partition GUIDs when the cmdline needs them.
> 49936b4c0109 libavb: Support vbmeta blobs in beginning of partition.
> 6f4de3181429 libavb: Add ERROR_MODE_MANAGED_RESTART_AND_EIO for managing dm-verity state.
> 2367b46287bd Implement initialization of new persistent digest values
>
> [3] avb (master) git diff --shortstat \
>     android-o-mr1-iot-preview-8..5fbb42a189aa -- libavb
>  12 files changed, 704 insertions(+), 208 deletions(-)
>
> [4] https://gitlab.denx.de/u-boot/u-boot/commit/ecc6f6bea6a2
>     ("libavb: Handle wrong hashtree_error_mode in avb_append_options()")
>
> --
> Best Regards,
> Eugeniu.

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Sam Protsenko]

Hi Eugeniu,

On Tue, Aug 13, 2019 at 7:59 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Fri, Aug 09, 2019 at 07:16:03PM +0300, Sam Protsenko wrote:
> > The requested_partitions[] array should contain only boot partitions.
> > Usually it's only 'boot' partition, as can be seen in [1]. Also, seems
> > like the requested_partitions[] are only used when there is no 'vbmeta'
> > partition [2], which is not a regular use-case.
> >
> > Make requested_partitions[] contain only 'boot' partition as it was
> > supposed to be, and also make that array to be a local in
> > do_avb_verify_part() function, as nobody else needs that.
> >
> > [1] https://android.googlesource.com/platform/external/avb/+/master/test/avb_slot_verify_unittest.cc#108
> > [2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_slot_verify.c#1461
>
> The patches are much appreciated. Could we agree to avoid volatile
> references in the links, since those will point out to wrong lines after
> a couple of weeks? I think it's safer to either use the latest available
> commit id or tag, e.g.:
>
> [1] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/test/avb_slot_verify_unittest.cc#108
> [2] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/libavb/avb_slot_verify.c#1461
>

Sure, will send v2 soon. Can you please review this patch [1] please?
Without this AVB doesn't work (at least on X15 board, but I presume it
might affect more platforms, as code I'm fixing in that patch is
common). Also I send [2] for slots support in AVB. Will appreciate
your review.

Thanks!

[1] https://patchwork.ozlabs.org/patch/1147191/
[2] https://patchwork.ozlabs.org/patch/1144646/

> Thank you.
>
> --
> Best Regards,
> Eugeniu.

[U-Boot] [PATCH] cmd: avb: Fix requested partitions list

[Igor Opaniuk]

On Thu, Aug 15, 2019 at 8:55 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> Hi Eugeniu,
>
> On Tue, Aug 13, 2019 at 7:59 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hi Sam,
> >
> > On Fri, Aug 09, 2019 at 07:16:03PM +0300, Sam Protsenko wrote:
> > > The requested_partitions[] array should contain only boot partitions.
> > > Usually it's only 'boot' partition, as can be seen in [1]. Also, seems
> > > like the requested_partitions[] are only used when there is no 'vbmeta'
> > > partition [2], which is not a regular use-case.
> > >
> > > Make requested_partitions[] contain only 'boot' partition as it was
> > > supposed to be, and also make that array to be a local in
> > > do_avb_verify_part() function, as nobody else needs that.
> > >
> > > [1] https://android.googlesource.com/platform/external/avb/+/master/test/avb_slot_verify_unittest.cc#108
> > > [2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_slot_verify.c#1461
> >
> > The patches are much appreciated. Could we agree to avoid volatile
> > references in the links, since those will point out to wrong lines after
> > a couple of weeks? I think it's safer to either use the latest available
> > commit id or tag, e.g.:
> >
> > [1] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/test/avb_slot_verify_unittest.cc#108
> > [2] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/libavb/avb_slot_verify.c#1461
> >
>
> Sure, will send v2 soon. Can you please review this patch [1] please?
> Without this AVB doesn't work (at least on X15 board, but I presume it
> might affect more platforms, as code I'm fixing in that patch is
> common). Also I send [2] for slots support in AVB. Will appreciate
> your review.
>
> Thanks!
>
> [1] https://patchwork.ozlabs.org/patch/1147191/
> [2] https://patchwork.ozlabs.org/patch/1144646/
>
> > Thank you.
> >
> > --
> > Best Regards,
> > Eugeniu.

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

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

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk