From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4BD77C433FE for ; Wed, 12 Oct 2022 12:48:42 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C538184E37; Wed, 12 Oct 2022 14:48:39 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20210112.gappssmtp.com header.i=@baylibre-com.20210112.gappssmtp.com header.b="lDpXSDzJ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 999E284E44; Wed, 12 Oct 2022 14:48:38 +0200 (CEST) Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4E03384DB3 for ; Wed, 12 Oct 2022 14:48:33 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@baylibre.com Received: by mail-wr1-x42b.google.com with SMTP id bp11so13336553wrb.9 for ; Wed, 12 Oct 2022 05:48:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=WaZ/wiB1JTi5caFtC7ROwFompXmVyvAmry5wshQ+kBA=; b=lDpXSDzJ5CU7QQokhOYgxdwVN8faYA0u6lg9AUqmj26Lpz0jgc7ipgWftde4yKmWzt 1wnf7wr8p9ECuQyEC3W3blD6PM4WNPgAeNoklYCAMLziDNV1eQA+owXUV7YRykwhZMCl be5S9bsULRWO4bi72g6Y28r28zmNzEtAp+pwBRjidEhv6TZd3wIkBlQKVIeVaayM1x88 YHHstyulDj/M7xgmyEjxC5H9fu7iJFh+T/X2tmce7QgaLcp3R/DQY6F/aN0TJ1FOaaKx EV7gt5EH24viNxWqIG1JYrDX1EDO1a98++OjdTOjmDEGMg9ePn83jd5pBu9Sd5qgfq/i q5+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WaZ/wiB1JTi5caFtC7ROwFompXmVyvAmry5wshQ+kBA=; b=PHRhX3Sx3g2uOBEd2Xsg7FQLhN77DGn03RgJCz6itbKiwzjwDytcIHR3E8caRFdNR2 xH/KVpmFsUksXIFzPrRCAWplDj01CZJiVYmkJxbomkfMARDNXaF08jFu5lWokGfrbz+i cgdKl/I9RcT6brB6p/QO5DUdSSPrd3Pi7EnAe2Q9xUqSXzXE5N4LxnpfyoYWg0xOHc/Y soHpjUW2vSEgMqFrGp5srGctNfNMFT6lc5qTa9xgXq+I5LC3tqfrBVsvHKhZq5ci7Bj0 c6B1MDzu6iNaQY8huMP1CFdVe355cFWpzLkMgAlOeR/3JNIryq4efADGDLeJXcf9+y7z Dmcw== X-Gm-Message-State: ACrzQf2b5PhFYOrWd6o6lZPUUr8LgjrtXoif9mWEgXhaCoQVTTYrZrkQ vgyjyGH3by+gIqkn8NNgpnHjgAl6wh3UVw== X-Google-Smtp-Source: AMsMyM6w94j2vOxoOcq8PnJ0zFbAfx/B2C6Er4p5L4ebT/SFbI5v89EBUJ4kR1F13qvoGJYsR5v7ow== X-Received: by 2002:a05:6000:1106:b0:22e:3dee:9a5a with SMTP id z6-20020a056000110600b0022e3dee9a5amr17870953wrw.191.1665578912778; Wed, 12 Oct 2022 05:48:32 -0700 (PDT) Received: from localhost ([2a01:cb19:85e6:1900:efdb:912:a64:6681]) by smtp.gmail.com with ESMTPSA id p13-20020a05600c468d00b003c6d8dd345bsm465431wmo.33.2022.10.12.05.48.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Oct 2022 05:48:31 -0700 (PDT) From: Mattijs Korpershoek To: Alistair Delva Cc: u-boot@lists.denx.de, Igor Opaniuk , Ram Muthiah , Jiyong Park , Simon Glass Subject: Re: [PATCH] avb: Extend support to non-eMMC interfaces In-Reply-To: References: <20220926220211.868968-1-adelva@google.com> <87o7ujpw6t.fsf@baylibre.com> Date: Wed, 12 Oct 2022 14:48:31 +0200 Message-ID: <87fsftdx7k.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On mar., oct. 11, 2022 at 13:40, Alistair Delva wrote: > On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek > wrote: >> >> Hi Alistair, >> >> Thank you for your patch. >> >> On lun., sept. 26, 2022 at 22:02, Alistair Delva wrote: >> >> > From: Jiyong Park >> > >> > 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 >> > Cc: Igor Opaniuk >> > Cc: Ram Muthiah >> > Cc: Jiyong Park >> > Cc: Simon Glass >> > --- >> > 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 >> > #include >> > #include >> > -#include >> > >> > #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 - initialize avb2 for \n" >> > + "init - initialize avb2 for the disk which\n" >> > + " is on the interface \n" >> > "avb read_rb - read rollback index at location \n" >> > "avb write_rb - write rollback index to \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 "#\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 >> > #include >> > -#include >> > +#include >> > >> > #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