[PATCH] avb: Extend support to non-eMMC interfaces
Mattijs Korpershoek
mkorpershoek at baylibre.com
Wed Oct 12 14:48:31 CEST 2022
On mar., oct. 11, 2022 at 13:40, Alistair Delva <adelva at google.com> wrote:
> On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek
> <mkorpershoek at baylibre.com> wrote:
>>
>> Hi Alistair,
>>
>> Thank you for your patch.
>>
>> On lun., sept. 26, 2022 at 22:02, Alistair Delva <adelva at google.com> wrote:
>>
>> > From: Jiyong Park <jiyong at google.com>
>> >
>> > Previously Android AVB supported block devices only on eMMC. This change
>> > eliminates the restriction by using the generic block driver model.
>> >
>> > The `avb init' command is modified to accept another parameter which
>> > specifies the interface type. e.g., `avb init virtio 0' initializes
>> > avb for the first (0) disk that is accessible via the virtio interface.
>> >
>> > [adelva: The "avb init" command is updated directly, as this is
>> > considered a "debug command" that can't be usefully used in u-boot
>> > scripts.]
>>
>> It seems that:
>> * include/configs/ti_omap5_common.h
>> * include/configs/meson64_android.h
>> * configs/total_compute_defconfig
>>
>> All call "avb init"
>>
>> Aren't we breaking these boot scripts with this change?
>>
>> Since all of them used mmc before, it should be trivial to patch these
>> environments as well.
>> If you do so, please cc me in next version so I can give this a try on
>> the khadas vim3l board.
>
> Sure, I'll do that and send a v2.
>
>> Maybe those devices are doing the wrong thing though. since this is
>> considered a debug command I imagine we should not be calling this at
>> all.
>> If so, do you have any alternatives in mind?
>
> Yes, sorry. My rationale was confusing. We have more patches to
> upstream which will explain the reasoning better:
>
> "data from boot and vendor_boot partitions are loaded only once for
> the verification. After the verification is done, the read data is
> directly copied to the load addresses instead of doing the I/O again
> from the disk. This is to prevent a TOCTOU issue where attacker provides
> different data for verification and actual booting."
Indeed. Thank you for the details. I understand now.
>
> So yes, what these env files do for now isn't safe, but there isn't a
> good upstream alternative. Anyway, this problem isn't related to what
> this patch is doing. So, I should update them for now.
ACK.
>
>> >
>> > Signed-off-by: Alistair Delva <adelva at google.com>
>> > Cc: Igor Opaniuk <igor.opaniuk at gmail.com>
>> > Cc: Ram Muthiah <rammuthiah at google.com>
>> > Cc: Jiyong Park <jiyong at google.com>
>> > Cc: Simon Glass <sjg at chromium.org>
>> > ---
>> > cmd/avb.c | 16 ++++---
>> > common/Kconfig | 1 -
>> > common/avb_verify.c | 105 +++++++++++++++++++++----------------------
>> > include/avb_verify.h | 31 ++++---------
>>
>> Should we also update the documentation in doc/android/avb2.rst ?
>
> Will do.
>
>> I also think this might break:
>> test/py/tests/test_android/test_avb.py
>
> I'll update it.
>
>> > 4 files changed, 69 insertions(+), 84 deletions(-)
>> >
>> > diff --git a/cmd/avb.c b/cmd/avb.c
>> > index 783f51b816..8bffe49011 100644
>> > --- a/cmd/avb.c
>> > +++ b/cmd/avb.c
>> > @@ -10,24 +10,25 @@
>> > #include <env.h>
>> > #include <image.h>
>> > #include <malloc.h>
>> > -#include <mmc.h>
>> >
>> > #define AVB_BOOTARGS "avb_bootargs"
>> > static struct AvbOps *avb_ops;
>> >
>> > int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>> > {
>> > - unsigned long mmc_dev;
>> > + const char *iface;
>> > + const char *devnum;
>> >
>> > - if (argc != 2)
>> > + if (argc != 3)
>> > return CMD_RET_USAGE;
>> >
>> > - mmc_dev = hextoul(argv[1], NULL);
>> > + iface = argv[1];
>> > + devnum = argv[2];
>> >
>> > if (avb_ops)
>> > avb_ops_free(avb_ops);
>> >
>> > - avb_ops = avb_ops_alloc(mmc_dev);
>> > + avb_ops = avb_ops_alloc(iface, devnum);
>> > if (avb_ops)
>> > return CMD_RET_SUCCESS;
>> >
>> > @@ -419,7 +420,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, "", ""),
>> > @@ -455,7 +456,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> <devnum> - initialize avb2 for the disk <devnum> which\n"
>> > + " is on the interface <interface>\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 ebee856e56..a66060767c 100644
>> > --- a/common/Kconfig
>> > +++ b/common/Kconfig
>> > @@ -703,7 +703,6 @@ config HASH
>> > config AVB_VERIFY
>> > bool "Build Android Verified Boot operations"
>> > depends on LIBAVB
>> > - depends on MMC
>> > depends on PARTITION_UUIDS
>> > help
>> > This option enables compilation of bootloader-dependent operations,
>> > diff --git a/common/avb_verify.c b/common/avb_verify.c
>> > index 0520a71455..d30bbb5726 100644
>> > --- a/common/avb_verify.c
>> > +++ b/common/avb_verify.c
>> > @@ -253,10 +253,10 @@ char *avb_set_enforce_verity(const char *cmdline)
>> >
>> > /**
>> > * ============================================================================
>> > - * IO(mmc) auxiliary functions
>> > + * IO auxiliary functions
>> > * ============================================================================
>> > */
>> > -static unsigned long mmc_read_and_flush(struct mmc_part *part,
>> > +static unsigned long blk_read_and_flush(struct avb_part *part,
>> > lbaint_t start,
>> > lbaint_t sectors,
>> > void *buffer)
>> > @@ -291,7 +291,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);
>> > @@ -302,7 +302,7 @@ 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,
>> > +static unsigned long blk_write(struct avb_part *part, lbaint_t start,
>> > lbaint_t sectors, void *buffer)
>> > {
>> > void *tmp_buf;
>> > @@ -330,69 +330,59 @@ 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)
>> > +static struct avb_part *get_partition(AvbOps *ops, const char *partition)
>> > {
>> > - int ret;
>> > - u8 dev_num;
>> > - int part_num = 0;
>> > - struct mmc_part *part;
>> > - struct blk_desc *mmc_blk;
>> > + struct avb_part *part;
>> > + struct AvbOpsData *data;
>> > + size_t dev_part_str_len;
>> > + char *dev_part_str;
>> >
>> > - part = malloc(sizeof(struct mmc_part));
>> > + part = malloc(sizeof(struct avb_part));
>> > if (!part)
>> > return NULL;
>> >
>> > - dev_num = get_boot_device(ops);
>> > - part->mmc = find_mmc_device(dev_num);
>> > - if (!part->mmc) {
>> > - printf("No MMC device at slot %x\n", dev_num);
>> > - goto err;
>> > - }
>> > -
>> > - if (mmc_init(part->mmc)) {
>> > - printf("MMC initialization failed\n");
>> > - goto err;
>> > - }
>> > + if (!ops)
>> > + return NULL;
>> >
>> > - ret = mmc_switch_part(part->mmc, part_num);
>> > - if (ret)
>> > - goto err;
>> > + data = ops->user_data;
>> > + if (!data)
>> > + return NULL;
>> >
>> > - mmc_blk = mmc_get_blk_desc(part->mmc);
>> > - if (!mmc_blk) {
>> > - printf("Error - failed to obtain block descriptor\n");
>> > - goto err;
>> > + // format is "<devnum>#<partition>\0"
>> > + dev_part_str_len = strlen(data->devnum) + 1 + strlen(partition) + 1;
>> > + dev_part_str = (char *)malloc(dev_part_str_len);
>> > + if (!dev_part_str) {
>> > + free(part);
>> > + return NULL;
>> > }
>> >
>> > - ret = part_get_info_by_name(mmc_blk, partition, &part->info);
>> > - if (ret < 0) {
>> > - printf("Can't find partition '%s'\n", partition);
>> > - goto err;
>> > + snprintf(dev_part_str, dev_part_str_len, "%s#%s", data->devnum,
>> > + partition);
>> > + if (part_get_info_by_dev_and_name_or_num(data->iface, dev_part_str,
>> > + &part->blk, &part->info,
>> > + false) < 0) {
>> > + free(part);
>> > + part = NULL;
>> > }
>> >
>> > - part->dev_num = dev_num;
>> > - part->mmc_blk = mmc_blk;
>> > -
>> > + free(dev_part_str);
>> > return part;
>> > -err:
>> > - free(part);
>> > - return NULL;
>> > }
>> >
>> > -static AvbIOResult mmc_byte_io(AvbOps *ops,
>> > +static AvbIOResult blk_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)
>> > + enum io_type io_type)
>> > {
>> > ulong ret;
>> > - struct mmc_part *part;
>> > + struct avb_part *part;
>> > u64 start_offset, start_sector, sectors, residue;
>> > u8 *tmp_buf;
>> > size_t io_cnt = 0;
>> > @@ -425,7 +415,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
>> > }
>> >
>> > if (io_type == IO_READ) {
>> > - ret = mmc_read_and_flush(part,
>> > + ret = blk_read_and_flush(part,
>> > part->info.start +
>> > start_sector,
>> > 1, tmp_buf);
>> > @@ -442,7 +432,7 @@ 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,
>> > + ret = blk_read_and_flush(part,
>> > part->info.start +
>> > start_sector,
>> > 1, tmp_buf);
>> > @@ -456,7 +446,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
>> > start_offset % part->info.blksz,
>> > buffer, residue);
>> >
>> > - ret = mmc_write(part, part->info.start +
>> > + ret = blk_write(part, part->info.start +
>> > start_sector, 1, tmp_buf);
>> > if (ret != 1) {
>> > printf("%s: write error (%ld, %lld)\n",
>> > @@ -474,12 +464,12 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
>> >
>> > if (sectors) {
>> > if (io_type == IO_READ) {
>> > - ret = mmc_read_and_flush(part,
>> > + ret = blk_read_and_flush(part,
>> > part->info.start +
>> > start_sector,
>> > sectors, buffer);
>> > } else {
>> > - ret = mmc_write(part,
>> > + ret = blk_write(part,
>> > part->info.start +
>> > start_sector,
>> > sectors, buffer);
>> > @@ -535,7 +525,7 @@ static AvbIOResult read_from_partition(AvbOps *ops,
>> > void *buffer,
>> > size_t *out_num_read)
>> > {
>> > - return mmc_byte_io(ops, partition_name, offset_from_partition,
>> > + return blk_byte_io(ops, partition_name, offset_from_partition,
>> > num_bytes, buffer, out_num_read, IO_READ);
>> > }
>> >
>> > @@ -562,7 +552,7 @@ static AvbIOResult write_to_partition(AvbOps *ops,
>> > size_t num_bytes,
>> > const void *buffer)
>> > {
>> > - return mmc_byte_io(ops, partition_name, offset_from_partition,
>> > + return blk_byte_io(ops, partition_name, offset_from_partition,
>> > num_bytes, (void *)buffer, NULL, IO_WRITE);
>> > }
>> >
>> > @@ -803,7 +793,7 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>> > char *guid_buf,
>> > size_t guid_buf_size)
>> > {
>> > - struct mmc_part *part;
>> > + struct avb_part *part;
>> > size_t uuid_size;
>> >
>> > part = get_partition(ops, partition);
>> > @@ -837,7 +827,7 @@ static AvbIOResult get_size_of_partition(AvbOps *ops,
>> > const char *partition,
>> > u64 *out_size_num_bytes)
>> > {
>> > - struct mmc_part *part;
>> > + struct avb_part *part;
>> >
>> > if (!out_size_num_bytes)
>> > return AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE;
>> > @@ -976,7 +966,7 @@ free_name:
>> > * AVB2.0 AvbOps alloc/initialisation/free
>> > * ============================================================================
>> > */
>> > -AvbOps *avb_ops_alloc(int boot_device)
>> > +AvbOps *avb_ops_alloc(const char *iface, const char *devnum)
>> > {
>> > struct AvbOpsData *ops_data;
>> >
>> > @@ -999,7 +989,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->iface = avb_strdup(iface);
>> > + ops_data->devnum = avb_strdup(devnum);
>> >
>> > return &ops_data->ops;
>> > }
>> > @@ -1018,6 +1009,12 @@ void avb_ops_free(AvbOps *ops)
>> > if (ops_data->tee)
>> > tee_close_session(ops_data->tee, ops_data->session);
>> > #endif
>> > + if (ops_data->iface)
>> > + avb_free((void*)ops_data->iface);
>> > +
>> > + if (ops_data->devnum)
>> > + avb_free((void*)ops_data->devnum);
>> > +
>> > avb_free(ops_data);
>> > }
>> > }
>> > diff --git a/include/avb_verify.h b/include/avb_verify.h
>> > index 1e787ba666..732839f74b 100644
>> > --- a/include/avb_verify.h
>> > +++ b/include/avb_verify.h
>> > @@ -9,8 +9,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"
>> > @@ -26,7 +27,8 @@ enum avb_boot_state {
>> >
>> > struct AvbOpsData {
>> > struct AvbOps ops;
>> > - int mmc_dev;
>> > + const char *iface;
>> > + const char *devnum;
>> > enum avb_boot_state boot_state;
>> > #ifdef CONFIG_OPTEE_TA_AVB
>> > struct udevice *tee;
>> > @@ -34,19 +36,17 @@ struct AvbOpsData {
>> > #endif
>> > };
>> >
>> > -struct mmc_part {
>> > - int dev_num;
>> > - struct mmc *mmc;
>> > - struct blk_desc *mmc_blk;
>> > +struct avb_part {
>> > + struct blk_desc *blk;
>> > struct disk_partition info;
>> > };
>> >
>> > -enum mmc_io_type {
>> > +enum io_type {
>> > IO_READ,
>> > IO_WRITE
>> > };
>> >
>> > -AvbOps *avb_ops_alloc(int boot_device);
>> > +AvbOps *avb_ops_alloc(const char *iface, const char *devnum);
>> > void avb_ops_free(AvbOps *ops);
>> >
>> > char *avb_set_state(AvbOps *ops, enum avb_boot_state boot_state);
>> > @@ -60,7 +60,7 @@ char *append_cmd_line(char *cmdline_orig, char *cmdline_new);
>> > * 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_part *part, int64_t offset)
>> > {
>> > u64 part_size = part->info.size * part->info.blksz;
>> >
>> > @@ -85,17 +85,4 @@ static inline bool is_buf_unaligned(void *buffer)
>> > return (bool)((uintptr_t)buffer % ALLOWED_BUF_ALIGN);
>> > }
>> >
>> > -static inline int get_boot_device(AvbOps *ops)
>> > -{
>> > - struct AvbOpsData *data;
>> > -
>> > - if (ops) {
>> > - data = ops->user_data;
>> > - if (data)
>> > - return data->mmc_dev;
>> > - }
>> > -
>> > - return -1;
>> > -}
>> > -
>> > #endif /* _AVB_VERIFY_H */
>> > --
>> > 2.30.2
More information about the U-Boot
mailing list