On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek
<mkorpersh...@baylibre.com> wrote:
>
> Hi Alistair,
>
> Thank you for your patch.
>
> On lun., sept. 26, 2022 at 22:02, Alistair Delva <ade...@google.com> wrote:
>
> > From: Jiyong Park <jiy...@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."
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.
> >
> > Signed-off-by: Alistair Delva <ade...@google.com>
> > Cc: Igor Opaniuk <igor.opan...@gmail.com>
> > Cc: Ram Muthiah <rammuth...@google.com>
> > Cc: Jiyong Park <jiy...@google.com>
> > Cc: Simon Glass <s...@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
