AVB assumed its partitions were on mmc: avb_ops_alloc() took a bare device number and get_partition() called find_mmc_device(). bootmeth_android accepts any bootdev but passes only desc->devnum, so an Android bootflow found on nvme 0 or usb 0 is verified against mmc 0.
Carry a uclass id alongside the device number and resolve the block device with blk_get_devnum_by_uclass_id(), keeping mmc_init() and the hardware partition switch on the mmc path only. "avb init <dev>" still means mmc, "avb init <interface> <dev>" selects another bus, and both now look the device up before allocating. get_boot_interface() returns UCLASS_INVALID without ops, so a caller that skipped "avb init" fails rather than reading mmc. The IO helpers and struct mmc_part only ever used struct blk_desc, so rename them avb_blk_read(), avb_blk_write() and struct avb_blk_part. Signed-off-by: Jan Kehren <[email protected]> --- Prior art: this is the third attempt at the same change. Both earlier ones stalled without a design objection being raised. https://lore.kernel.org/u-boot/[email protected]/ https://lore.kernel.org/u-boot/[email protected]/ The first carries Reviewed-by: Simon Glass. Review asked for the in-tree "avb init" callers to be updated, plus docs and tests, and the v2 never came. That version made the interface argument mandatory, which is what broke the callers; keeping "avb init <dev>" as mmc avoids the problem, so no caller changes and board/ti/am57xx/am57xx.env keeps working as is. AVB_VERIFY keeps its "depends on MMC": the mmc bring-up path still calls into the mmc core. Dropping it needs IS_ENABLED(CONFIG_MMC) guards and is left as a separate change. "avb init" still parses <dev> as hex, as it always has, and the help text now says so. Happy to switch the new interface form to decimal to match every other block command if that is preferred. Tested on Raspberry Pi 4 and Raspberry Pi 5, verifying and booting Android from SD on both and from USB mass storage on the Pi 4. Those boards boot through a downstream command rather than bootstd, so the bootmeth_android hunk is build tested only. On the Pi 4 "avb init usb 0" then "avb verify _a" passes, "avb init 0" still resolves to mmc, and an unknown interface or an out-of-range index on either bus is rejected without disturbing ops that were already initialised. Build tested with khadas-vim3_android_defconfig and khadas-vim3l_android_ab_defconfig, both AVB_VERIFY=y CMD_AVB=y BOOTMETH_ANDROID=y. checkpatch --strict is clean apart from two CamelCase notes on libavb's own AvbOps/AvbIOResult types. boot/bootmeth_android.c | 2 +- cmd/avb.c | 24 +++- common/Kconfig | 7 +- common/avb_verify.c | 168 ++++++++++++++----------- doc/android/avb2.rst | 7 +- include/avb_verify.h | 38 ++++-- test/py/tests/test_android/test_avb.py | 28 +++++ 7 files changed, 182 insertions(+), 92 deletions(-) diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c index 0db08d4f86..ae05c08488 100644 --- a/boot/bootmeth_android.c +++ b/boot/bootmeth_android.c @@ -443,7 +443,7 @@ static int run_avb_verification(struct bootflow *bflow) bool unlocked = false; int ret; - avb_ops = avb_ops_alloc(desc->devnum); + avb_ops = avb_ops_alloc(desc->uclass_id, desc->devnum); if (!avb_ops) return log_msg_ret("avb ops", -ENOMEM); diff --git a/cmd/avb.c b/cmd/avb.c index 8fbd48ee5a..9381c2aa68 100644 --- a/cmd/avb.c +++ b/cmd/avb.c @@ -16,17 +16,28 @@ static struct AvbOps *avb_ops; int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { - unsigned long mmc_dev; + struct blk_desc *desc; - if (argc != 2) + if (argc < 2 || argc > 3) return CMD_RET_USAGE; - mmc_dev = hextoul(argv[1], NULL); + /* "avb init <dev>" keeps its old meaning of mmc <dev> */ + if (argc == 3) + desc = blk_get_devnum_by_uclass_idname(argv[1], + hextoul(argv[2], NULL)); + else + desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, + hextoul(argv[1], NULL)); + + if (!desc) { + printf("Can't find the requested device\n"); + return CMD_RET_FAILURE; + } if (avb_ops) avb_ops_free(avb_ops); - avb_ops = avb_ops_alloc(mmc_dev); + avb_ops = avb_ops_alloc(desc->uclass_id, desc->devnum); if (avb_ops) return CMD_RET_SUCCESS; else @@ -428,7 +439,7 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc, } static struct cmd_tbl cmd_avb[] = { - U_BOOT_CMD_MKENT(init, 2, 0, do_avb_init, "", ""), + U_BOOT_CMD_MKENT(init, 3, 0, do_avb_init, "", ""), U_BOOT_CMD_MKENT(read_rb, 2, 0, do_avb_read_rb, "", ""), U_BOOT_CMD_MKENT(write_rb, 3, 0, do_avb_write_rb, "", ""), U_BOOT_CMD_MKENT(is_unlocked, 1, 0, do_avb_is_unlocked, "", ""), @@ -464,7 +475,8 @@ static int do_avb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) U_BOOT_CMD( avb, 29, 0, do_avb, "Provides commands for testing Android Verified Boot 2.0 functionality", - "init <dev> - initialize avb2 for <dev>\n" + "init [<interface>] <dev> - initialize avb2 for hex <dev> on <interface>\n" + " (mmc, nvme, usb, ...; defaults to mmc)\n" "avb read_rb <num> - read rollback index at location <num>\n" "avb write_rb <num> <rb> - write rollback index <rb> to <num>\n" "avb is_unlocked - returns unlock status of the device\n" diff --git a/common/Kconfig b/common/Kconfig index c739225621..5590fbfdb2 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -885,9 +885,14 @@ config AVB_VERIFY This option enables compilation of bootloader-dependent operations, used by Android Verified Boot 2.0 library (libavb). Includes: * Helpers to process strings in order to build OS bootargs. - * Helpers to access MMC, similar to drivers/fastboot/fb_mmc.c. + * Helpers to access block devices, similar to + drivers/fastboot/fb_mmc.c. * Helpers to alloc/init/free avb ops. + Only mmc is initialised by AVB itself. Any other interface must + already be enumerated when AVB runs, either by the board code or by + a command such as "usb start". + if AVB_VERIFY config AVB_BUF_ADDR diff --git a/common/avb_verify.c b/common/avb_verify.c index b96cd1ebd7..6be34db129 100644 --- a/common/avb_verify.c +++ b/common/avb_verify.c @@ -8,6 +8,7 @@ #include <cpu_func.h> #include <image.h> #include <malloc.h> +#include <mmc.h> #include <part.h> #include <tee.h> #include <tee/optee_ta_avb.h> @@ -304,13 +305,13 @@ char *avb_set_enforce_verity(const char *cmdline) /** * ============================================================================ - * IO(mmc) auxiliary functions + * Block IO auxiliary functions * ============================================================================ */ -static unsigned long mmc_read_and_flush(struct mmc_part *part, - lbaint_t start, - lbaint_t sectors, - void *buffer) +static unsigned long avb_blk_read(struct avb_blk_part *part, + lbaint_t start, + lbaint_t sectors, + void *buffer) { unsigned long blks; void *tmp_buf; @@ -342,7 +343,7 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part, tmp_buf = buffer; } - blks = blk_dread(part->mmc_blk, + blks = blk_dread(part->blk, start, sectors, tmp_buf); /* flush cache after read */ flush_cache((ulong)tmp_buf, sectors * part->info.blksz); @@ -353,8 +354,8 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part, return blks; } -static unsigned long mmc_write(struct mmc_part *part, lbaint_t start, - lbaint_t sectors, void *buffer) +static unsigned long avb_blk_write(struct avb_blk_part *part, lbaint_t start, + lbaint_t sectors, void *buffer) { void *tmp_buf; size_t buf_size; @@ -382,59 +383,83 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start, tmp_buf = buffer; } - return blk_dwrite(part->mmc_blk, + return blk_dwrite(part->blk, start, sectors, tmp_buf); } -static struct mmc_part *get_partition(AvbOps *ops, const char *partition) +/* Hardware partition 0 is the user area, where the GPT and our images live */ +static int avb_init_mmc(int dev_num) { - int ret; - u8 dev_num; int part_num = 0; - struct mmc_part *part; - struct blk_desc *mmc_blk; - - part = malloc(sizeof(struct mmc_part)); - if (!part) - return NULL; + struct mmc *mmc; + int ret; - dev_num = get_boot_device(ops); - part->mmc = find_mmc_device(dev_num); - if (!part->mmc) { + mmc = find_mmc_device(dev_num); + if (!mmc) { printf("%s: no MMC device at slot %x\n", __func__, dev_num); - goto err; + return -ENODEV; } - ret = mmc_init(part->mmc); + ret = mmc_init(mmc); if (ret) { printf("%s: MMC initialization failed, err = %d\n", __func__, ret); - goto err; + return ret; } - if (IS_MMC(part->mmc)) { - ret = mmc_switch_part(part->mmc, part_num); + if (IS_MMC(mmc)) { + ret = mmc_switch_part(mmc, part_num); if (ret) { printf("%s: MMC part switch failed, err = %d\n", __func__, ret); - goto err; + return ret; } } - mmc_blk = mmc_get_blk_desc(part->mmc); - if (!mmc_blk) { - printf("%s: failed to obtain block descriptor\n", __func__); + return 0; +} + +static struct avb_blk_part *get_partition(AvbOps *ops, const char *partition) +{ + int ret; + int dev_num; + enum uclass_id uclass_id; + struct avb_blk_part *part; + struct blk_desc *blk; + + part = malloc(sizeof(struct avb_blk_part)); + if (!part) + return NULL; + + dev_num = get_boot_device(ops); + uclass_id = get_boot_interface(ops); + if (uclass_id == UCLASS_INVALID || dev_num < 0) { + printf("%s: no storage interface, run 'avb init' first\n", + __func__); + goto err; + } + + if (uclass_id == UCLASS_MMC) { + ret = avb_init_mmc(dev_num); + if (ret) + goto err; + } + + blk = blk_get_devnum_by_uclass_id(uclass_id, dev_num); + if (!blk) { + /* Every bus but mmc must already be started when AVB runs */ + printf("%s: no %s device %d, is the bus started?\n", __func__, + blk_get_uclass_name(uclass_id), dev_num); goto err; } - ret = part_get_info_by_name(mmc_blk, partition, &part->info); + ret = part_get_info_by_name(blk, partition, &part->info); if (ret < 0) { printf("%s: can't find partition '%s'\n", __func__, partition); goto err; } - part->dev_num = dev_num; - part->mmc_blk = mmc_blk; + part->blk = blk; return part; err: @@ -442,22 +467,22 @@ err: return NULL; } -static AvbIOResult mmc_byte_io(AvbOps *ops, - const char *partition, - s64 offset, - size_t num_bytes, - void *buffer, - size_t *out_num_read, - enum mmc_io_type io_type) +static AvbIOResult avb_blk_byte_io(AvbOps *ops, + const char *partition, + s64 offset, + size_t num_bytes, + void *buffer, + size_t *out_num_read, + enum avb_io_type io_type) { ulong ret; - struct mmc_part *part; + struct avb_blk_part *part; u64 start_offset, start_sector, sectors, residue; u8 *tmp_buf; size_t io_cnt = 0; AvbIOResult io_ret = AVB_IO_RESULT_OK; - if (!partition || !buffer || io_type > IO_WRITE) + if (!partition || !buffer || io_type > AVB_IO_WRITE) return AVB_IO_RESULT_ERROR_IO; part = get_partition(ops, partition); @@ -486,11 +511,11 @@ static AvbIOResult mmc_byte_io(AvbOps *ops, residue = num_bytes; } - if (io_type == IO_READ) { - ret = mmc_read_and_flush(part, - part->info.start + - start_sector, - 1, tmp_buf); + if (io_type == AVB_IO_READ) { + ret = avb_blk_read(part, + part->info.start + + start_sector, + 1, tmp_buf); if (ret != 1) { printf("%s: read error (%ld, %lld)\n", @@ -505,10 +530,10 @@ static AvbIOResult mmc_byte_io(AvbOps *ops, tmp_buf += (start_offset % part->info.blksz); memcpy(buffer, (void *)tmp_buf, residue); } else { - ret = mmc_read_and_flush(part, - part->info.start + - start_sector, - 1, tmp_buf); + ret = avb_blk_read(part, + part->info.start + + start_sector, + 1, tmp_buf); if (ret != 1) { printf("%s: read error (%ld, %lld)\n", @@ -520,8 +545,8 @@ static AvbIOResult mmc_byte_io(AvbOps *ops, start_offset % part->info.blksz, buffer, residue); - ret = mmc_write(part, part->info.start + - start_sector, 1, tmp_buf); + ret = avb_blk_write(part, part->info.start + + start_sector, 1, tmp_buf); if (ret != 1) { printf("%s: write error (%ld, %lld)\n", __func__, ret, start_sector); @@ -538,16 +563,16 @@ static AvbIOResult mmc_byte_io(AvbOps *ops, } if (sectors) { - if (io_type == IO_READ) { - ret = mmc_read_and_flush(part, - part->info.start + - start_sector, - sectors, buffer); + if (io_type == AVB_IO_READ) { + ret = avb_blk_read(part, + part->info.start + + start_sector, + sectors, buffer); } else { - ret = mmc_write(part, - part->info.start + - start_sector, - sectors, buffer); + ret = avb_blk_write(part, + part->info.start + + start_sector, + sectors, buffer); } if (!ret) { @@ -564,7 +589,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops, } /* Set counter for read operation */ - if (io_type == IO_READ && out_num_read) + if (io_type == AVB_IO_READ && out_num_read) *out_num_read = io_cnt; out: @@ -603,8 +628,8 @@ static AvbIOResult read_from_partition(AvbOps *ops, void *buffer, size_t *out_num_read) { - return mmc_byte_io(ops, partition_name, offset_from_partition, - num_bytes, buffer, out_num_read, IO_READ); + return avb_blk_byte_io(ops, partition_name, offset_from_partition, + num_bytes, buffer, out_num_read, AVB_IO_READ); } /** @@ -630,8 +655,8 @@ static AvbIOResult write_to_partition(AvbOps *ops, size_t num_bytes, const void *buffer) { - return mmc_byte_io(ops, partition_name, offset_from_partition, - num_bytes, (void *)buffer, NULL, IO_WRITE); + return avb_blk_byte_io(ops, partition_name, offset_from_partition, + num_bytes, (void *)buffer, NULL, AVB_IO_WRITE); } /** @@ -883,7 +908,7 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops, char *guid_buf, size_t guid_buf_size) { - struct mmc_part *part; + struct avb_blk_part *part; size_t uuid_size; part = get_partition(ops, partition); @@ -920,7 +945,7 @@ static AvbIOResult get_size_of_partition(AvbOps *ops, const char *partition, u64 *out_size_num_bytes) { - struct mmc_part *part; + struct avb_blk_part *part; if (!out_size_num_bytes) return AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE; @@ -1111,7 +1136,7 @@ __weak AvbIOResult avb_read_root_key_digest(AvbOps *ops, uint8_t *digest) * AVB2.0 AvbOps alloc/initialisation/free * ============================================================================ */ -AvbOps *avb_ops_alloc(int boot_device) +AvbOps *avb_ops_alloc(enum uclass_id uclass_id, int dev_num) { struct AvbOpsData *ops_data; @@ -1134,7 +1159,8 @@ AvbOps *avb_ops_alloc(int boot_device) ops_data->ops.read_persistent_value = read_persistent_value; #endif ops_data->ops.get_size_of_partition = get_size_of_partition; - ops_data->mmc_dev = boot_device; + ops_data->uclass_id = uclass_id; + ops_data->dev_num = dev_num; return &ops_data->ops; } diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst index a514701241..cef49d3e0d 100644 --- a/doc/android/avb2.rst +++ b/doc/android/avb2.rst @@ -29,6 +29,10 @@ integration in ``common/avb_verify.c`` and the platform port under For additional details check [1]_. +Only mmc is started by AVB itself. Any other interface must already be +enumerated when AVB runs, either by the board code or by a command such as +``usb start``, or the partition lookup fails. + AVB using OP-TEE (optional) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -80,7 +84,8 @@ AVB 2.0 U-Boot shell commands Provides CLI interface to invoke AVB 2.0 verification + misc. commands for different testing purposes:: - avb init <dev> - initialize avb 2 for <dev> + avb init [<interface>] <dev> - initialize avb 2 for hex <dev> on + <interface> (mmc, nvme, usb, ...; defaults to mmc) avb read_rb <num> - read rollback index at location <num> avb write_rb <num> <rb> - write rollback index <rb> to <num> avb is_unlocked - returns unlock status of the device diff --git a/include/avb_verify.h b/include/avb_verify.h index 66655aab87..6dbbb39a8a 100644 --- a/include/avb_verify.h +++ b/include/avb_verify.h @@ -7,8 +7,9 @@ #define _AVB_VERIFY_H #include <../lib/libavb/libavb.h> +#include <blk.h> #include <mapmem.h> -#include <mmc.h> +#include <part.h> #define AVB_MAX_ARGS 1024 #define VERITY_TABLE_OPT_RESTART "restart_on_corruption" @@ -24,7 +25,8 @@ enum avb_boot_state { struct AvbOpsData { struct AvbOps ops; - int mmc_dev; + enum uclass_id uclass_id; + int dev_num; enum avb_boot_state boot_state; #ifdef CONFIG_OPTEE_TA_AVB struct udevice *tee; @@ -32,19 +34,17 @@ struct AvbOpsData { #endif }; -struct mmc_part { - int dev_num; - struct mmc *mmc; - struct blk_desc *mmc_blk; +struct avb_blk_part { + struct blk_desc *blk; struct disk_partition info; }; -enum mmc_io_type { - IO_READ, - IO_WRITE +enum avb_io_type { + AVB_IO_READ, + AVB_IO_WRITE }; -AvbOps *avb_ops_alloc(int boot_device); +AvbOps *avb_ops_alloc(enum uclass_id uclass_id, int dev_num); void avb_ops_free(AvbOps *ops); /** @@ -78,7 +78,7 @@ const char *str_avb_slot_error(AvbSlotVerifyResult res); * I/O helper inline functions * ============================================================================ */ -static inline uint64_t calc_offset(struct mmc_part *part, int64_t offset) +static inline uint64_t calc_offset(struct avb_blk_part *part, int64_t offset) { u64 part_size = part->info.size * part->info.blksz; @@ -110,10 +110,24 @@ static inline int get_boot_device(AvbOps *ops) if (ops) { data = ops->user_data; if (data) - return data->mmc_dev; + return data->dev_num; } return -1; } +/* Mirrors get_boot_device(): no ops means no interface, not a default one */ +static inline enum uclass_id get_boot_interface(AvbOps *ops) +{ + struct AvbOpsData *data; + + if (ops) { + data = ops->user_data; + if (data) + return data->uclass_id; + } + + return UCLASS_INVALID; +} + #endif /* _AVB_VERIFY_H */ diff --git a/test/py/tests/test_android/test_avb.py b/test/py/tests/test_android/test_avb.py index 137d83e1de..4c5b3c6f59 100644 --- a/test/py/tests/test_android/test_avb.py +++ b/test/py/tests/test_android/test_avb.py @@ -135,3 +135,31 @@ def test_avb_persistent_values(ubman): response = ubman.run_command('avb read_pvalue test 12') assert response == 'Read 12 bytes, value = value_value' + + [email protected]('cmd_avb') [email protected]('cmd_mmc') +def test_avb_init_interface(ubman): + """Check that 'avb init' accepts an explicit interface + """ + + response = ubman.run_command('avb init mmc %s' % str(mmc_dev)) + assert response == '' + + response = ubman.run_command('avb init %s' % str(mmc_dev)) + assert response == '' + + [email protected]('cmd_avb') +def test_avb_init_bad_device(ubman): + """Check that 'avb init' rejects a device that does not exist + """ + + response = ubman.run_command('avb init mmc ff') + assert response.find("Can't find the requested device") != -1 + + response = ubman.run_command('avb init ff') + assert response.find("Can't find the requested device") != -1 + + response = ubman.run_command('avb init nonsense 0') + assert response.find("Can't find the requested device") != -1 base-commit: 6e3f574e576585b0250ac387475d59e581bb5d9f -- 2.43.0
