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
