cmd: avb: free partition buffer upon verify completion

Allows to run 'avb verify' multiple times which can be useful after a
failure to be able to re-flash the partition and try again.

Signed-off-by: Gary Bisson <gary.bisson at boundarydevices.com>
---
Hi,

This was added because of the following scenario:
1- fastboot flash boot boot.img
2- avb verify
  -> fails because vbmeta wasn't updated
3- fastboot flash vbmeta vbmeta.img
4- avb verify
  -> fails because it can't allocate memory as previous buffer wasn't
freed

Let me know if you have any questions.

Regards,
Gary
---
 cmd/avb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi Gary,

Thanks for the patch and sorry for a late reply.

On Wed, May 6, 2020 at 5:06 PM Gary Bisson
<gary.bisson at boundarydevices.com> wrote:
>
> Allows to run 'avb verify' multiple times which can be useful after a
> failure to be able to re-flash the partition and try again.
>
> Signed-off-by: Gary Bisson <gary.bisson at boundarydevices.com>
> ---
> Hi,
>
> This was added because of the following scenario:
> 1- fastboot flash boot boot.img
> 2- avb verify
>   -> fails because vbmeta wasn't updated
> 3- fastboot flash vbmeta vbmeta.img
> 4- avb verify
>   -> fails because it can't allocate memory as previous buffer wasn't
> freed
>
> Let me know if you have any questions.
>
> Regards,
> Gary
> ---
>  cmd/avb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index a4de5c40a2..67154de4e1 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -312,6 +312,14 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
>                 printf("Unknown error occurred\n");
>         }
>
> +       /* Free image buffers now that verification is complete */
> +       if (slot_result != AVB_SLOT_VERIFY_RESULT_ERROR_OOM) {
Freeing out_data can be done not only when res is
AVB_SLOT_VERIFY_RESULT_ERROR_OOM.
For example, in the line 1196 in [1]:

AvbSlotVerifyResult sub_ret = avb_append_options(ops,
                                                 slot_data,
                                                 &toplevel_vbmeta,
                                                 algorithm_type,
                                                 hashtree_error_mode);
if (sub_ret != AVB_SLOT_VERIFY_RESULT_OK) {
        ret = sub_ret;
        goto fail;
}

> +               int i;
> +               for (i = 0; i < (int)out_data->num_loaded_partitions; i++)
> +                       if (out_data->loaded_partitions[i].data)
> +                               free(out_data->loaded_partitions[i].data);

It's better to use avb_slot_verify_data_free()  for the whole out_data instead
for proper cleanup. Check how it's done in unittests for libavb [2].

> +       }
> +
>         return res;
>  }
>
> --
> 2.26.2
>

[1] lib/libavb/avb_slot_verify.c
[2] https://android.googlesource.com/platform/external/avb/+/refs/tags/android-9.0.0_r37/test/avb_slot_verify_unittest.cc#156

Hi Igor,

On Sun, May 10, 2020 at 01:30:53PM +0300, Igor Opaniuk wrote:
> Hi Gary,
> 
> Thanks for the patch and sorry for a late reply.

No problem.

> On Wed, May 6, 2020 at 5:06 PM Gary Bisson
> <gary.bisson at boundarydevices.com> wrote:
> >
> > Allows to run 'avb verify' multiple times which can be useful after a
> > failure to be able to re-flash the partition and try again.
> >
> > Signed-off-by: Gary Bisson <gary.bisson at boundarydevices.com>
> > ---
> > Hi,
> >
> > This was added because of the following scenario:
> > 1- fastboot flash boot boot.img
> > 2- avb verify
> >   -> fails because vbmeta wasn't updated
> > 3- fastboot flash vbmeta vbmeta.img
> > 4- avb verify
> >   -> fails because it can't allocate memory as previous buffer wasn't
> > freed
> >
> > Let me know if you have any questions.
> >
> > Regards,
> > Gary
> > ---
> >  cmd/avb.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/cmd/avb.c b/cmd/avb.c
> > index a4de5c40a2..67154de4e1 100644
> > --- a/cmd/avb.c
> > +++ b/cmd/avb.c
> > @@ -312,6 +312,14 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
> >                 printf("Unknown error occurred\n");
> >         }
> >
> > +       /* Free image buffers now that verification is complete */
> > +       if (slot_result != AVB_SLOT_VERIFY_RESULT_ERROR_OOM) {
> Freeing out_data can be done not only when res is
> AVB_SLOT_VERIFY_RESULT_ERROR_OOM.
> For example, in the line 1196 in [1]:
> 
> AvbSlotVerifyResult sub_ret = avb_append_options(ops,
>                                                  slot_data,
>                                                  &toplevel_vbmeta,
>                                                  algorithm_type,
>                                                  hashtree_error_mode);
> if (sub_ret != AVB_SLOT_VERIFY_RESULT_OK) {
>         ret = sub_ret;
>         goto fail;
> }

Indeed.

> > +               int i;
> > +               for (i = 0; i < (int)out_data->num_loaded_partitions; i++)
> > +                       if (out_data->loaded_partitions[i].data)
> > +                               free(out_data->loaded_partitions[i].data);
> 
> It's better to use avb_slot_verify_data_free()  for the whole out_data instead
> for proper cleanup. Check how it's done in unittests for libavb [2].

I'll check it out and offer a v2. Thanks for your feedback.

Regards,
Gary