Re: [PATCH v1 2/2] boot: android: Add AVB verification support for different bootflow.

Hi Valentin,

On 2026-08-18T17:53:00, Valentin Liu <[email protected]> wrote:
> boot: android: Add AVB verification support for different bootflow.

Drop the trailing period from the subject.

>
> The different Android versions have their own partition layout,
> so the AVB verification process should be dynamic.
>
> In the new verification process, we need to use the header version
> fetched from boot partition, so we need to check the boot partition
> firstly to avoid the downgrade attacking.
>
> If we didn't check boot firstly, just use it, the attacker can
> bypass the AVB verification by flashing a boot image with header
> version 3 or earlier.

Please use present tense throughout, 'first' rather than 'firstly',
and 'downgrade attack' rather than 'downgrade attacking'. I also don't
follow the security argument: priv->header_version is populated by
scan_boot_part() from the unverified on-disk image and is not re-read
after the first AVB pass, so verifying boot on its own does not change
which partitions the second pass includes - an attacker who can flash
a signed v3 boot image would still get init_boot skipped. Can you
explain what the first pass actually protects against, or is the
intent to re-parse the header from the AVB-verified copy before the
second call?

>
> Signed-off-by: Valentin Liu <[email protected]>
>
> boot/bootmeth_android.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 5 deletions(-)

> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> @@ -479,11 +479,12 @@ static int avb_append_commandline(struct bootflow *bflow, char *cmdline)
>       return 0;
>  }
>
> -static int run_avb_verification(struct bootflow *bflow)
> +static int run_avb_verification(struct bootflow *bflow, const bool boot_only)

The const on a by-value parameter has no effect on callers and isn't
the style used elsewhere in this file - please drop it.

> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> @@ -493,6 +494,28 @@ static int run_avb_verification(struct bootflow *bflow)
>       bool unlocked = false;
>       int ret;
>
> +     /*
> +      * Always verify boot first.
> +      *
> +      * When boot_only is true, only verify the boot partition.
> +      * Otherwise, select additional partitions according to the
> +      * Android boot image header version.
> +      */
> +     requested_partitions[requested_partitions_num++] = "boot";
> +
> +     if (!boot_only) {
> +             if (priv->header_version >= 3)
> +                     requested_partitions[requested_partitions_num++] =
> +                             "vendor_boot";
> +
> +             if (priv->header_version >= 4 &&
> +                 priv->init_boot_img_size > 0)
> +                     requested_partitions[requested_partitions_num++] =
> +                             "init_boot";
> +     }
> +
> +     requested_partitions[requested_partitions_num] = NULL;

Please use BOOT_PART_NAME / VENDOR_BOOT_PART_NAME /
INIT_BOOT_PART_NAME instead of open-coded strings. Also 'n' or 'count'
would read better than requested_partitions_num, and the array bound 4
would benefit from a named constant or a comment for the boot +
vendor_boot + init_boot + NULL layout.

> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> @@ -562,9 +585,10 @@ static int run_avb_verification(struct bootflow *bflow)
>       return ret;
>  }
>  #else
> -static int run_avb_verification(struct bootflow *bflow)
> +static int run_avb_verification(struct bootflow *bflow, const bool boot_only)
>  {
>       int ret;
> +     (void)boot_only;

Can you use __maybe_unused on the parameter rather than a (void) cast?
That's the U-Boot convention for stubs.

> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> @@ -617,9 +641,13 @@ static int boot_android_normal(struct bootflow *bflow)
>       ulong iloadaddr = env_get_hex("init_boot_comp_addr_r", 0);
>       ulong vloadaddr = env_get_hex("vendor_boot_comp_addr_r", 0);
>
> -     ret = run_avb_verification(bflow);
> +     /*
> +      * Checking the boot partition firstly because the standard AVB
> +      * verification is rely on the header version from boot partition.
> +      */
> +     ret = run_avb_verification(bflow, true);

Grammar: 'Check the boot partition first, because the standard AVB
verification relies on the header version read from the boot
partition.' See my earlier comment - please clarify in the comment and
commit message what the first pass buys us, given that
priv->header_version is not re-read between the two calls.

> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> @@ -631,6 +659,11 @@ static int boot_android_normal(struct bootflow *bflow)
>       if (ret < 0)
>               return log_msg_ret("read boot", ret);
>
> +     /* Standard AVB verification */
> +     ret = run_avb_verification(bflow, false);
> +     if (ret < 0)
> +             return log_msg_ret("avb", ret);
> +

This re-verifies boot as well as vendor_boot/init_boot, so the boot
partition (which can be tens of MB) is hashed twice on every boot. Can
you split run_avb_verification() so the second call verifies only the
additional partitions? That would also avoid appending the same
androidboot.vbmeta/verifiedbootstate args twice and relying on
bootflow_cmdline_set_arg() to dedupe by key.

Regards,
Simon