| Message ID | 20220926220211.868968-1-adelva@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | avb: Extend support to non-eMMC interfaces | expand |
On Mon, 26 Sept 2022 at 16:02, Alistair Delva <adelva@google.com> wrote: > > From: Jiyong Park <jiyong@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.] > > Signed-off-by: Alistair Delva <adelva@google.com> > Cc: Igor Opaniuk <igor.opaniuk@gmail.com> > Cc: Ram Muthiah <rammuthiah@google.com> > Cc: Jiyong Park <jiyong@google.com> > Cc: Simon Glass <sjg@chromium.org> > --- > cmd/avb.c | 16 ++++--- > common/Kconfig | 1 - > common/avb_verify.c | 105 +++++++++++++++++++++---------------------- > include/avb_verify.h | 31 ++++--------- > 4 files changed, 69 insertions(+), 84 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Alistair, Thank you for your patch. On lun., sept. 26, 2022 at 22:02, Alistair Delva <adelva@google.com> wrote: > From: Jiyong Park <jiyong@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. 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? > > Signed-off-by: Alistair Delva <adelva@google.com> > Cc: Igor Opaniuk <igor.opaniuk@gmail.com> > Cc: Ram Muthiah <rammuthiah@google.com> > Cc: Jiyong Park <jiyong@google.com> > Cc: Simon Glass <sjg@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 ? I also think this might break: test/py/tests/test_android/test_avb.py > 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
On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote: > > Hi Alistair, > > Thank you for your patch. > > On lun., sept. 26, 2022 at 22:02, Alistair Delva <adelva@google.com> wrote: > > > From: Jiyong Park <jiyong@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 <adelva@google.com> > > Cc: Igor Opaniuk <igor.opaniuk@gmail.com> > > Cc: Ram Muthiah <rammuthiah@google.com> > > Cc: Jiyong Park <jiyong@google.com> > > Cc: Simon Glass <sjg@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
On mar., oct. 11, 2022 at 13:40, Alistair Delva <adelva@google.com> wrote: > On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek > <mkorpershoek@baylibre.com> wrote: >> >> Hi Alistair, >> >> Thank you for your patch. >> >> On lun., sept. 26, 2022 at 22:02, Alistair Delva <adelva@google.com> wrote: >> >> > From: Jiyong Park <jiyong@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@google.com> >> > Cc: Igor Opaniuk <igor.opaniuk@gmail.com> >> > Cc: Ram Muthiah <rammuthiah@google.com> >> > Cc: Jiyong Park <jiyong@google.com> >> > Cc: Simon Glass <sjg@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
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 */