| Message ID | 20190809161603.26055-1-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] cmd: avb: Fix requested partitions list | expand |
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
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()")
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.
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.
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.
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>
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;
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(-)