[U-Boot] [PATCH v2 1/1] avb: add support for named persistent values
Jens Wiklander
jens.wiklander at linaro.org
Mon Jan 14 09:26:01 UTC 2019
Hi Igor,
Some comments below.
On Fri, Dec 14, 2018 at 07:45:03PM +0200, Igor Opaniuk wrote:
> AVB version 1.1 introduces support for named persistent values
> that must be tamper evident and allows AVB to store arbitrary key-value
> pairs [1].
>
> Introduce implementation of two additional AVB operations
> read_persistent_value()/write_persistent_value() for retrieving/storing
> named persistent values.
>
> Correspondent pull request in the OP-TEE OS project repo [2].
>
> [1]: https://android.googlesource.com/platform/external/avb/+/android-9.0.0_r22
> [2]: https://github.com/OP-TEE/optee_os/pull/2699
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk at linaro.org>
> ---
>
> Changes in v2:
> - fix output format for avb read_pvalue/write_pvalue commands
> - fix issue with named value buffer size
>
> cmd/avb.c | 78 +++++++++++++++++++++++++++++
> common/avb_verify.c | 122 +++++++++++++++++++++++++++++++++++++++++++++
> include/tee.h | 2 +
> include/tee/optee_ta_avb.h | 16 ++++++
> 4 files changed, 218 insertions(+)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index ff00be4..8387cc7 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -340,6 +340,76 @@ int do_avb_is_unlocked(cmd_tbl_t *cmdtp, int flag,
> return CMD_RET_FAILURE;
> }
>
> +int do_avb_read_pvalue(cmd_tbl_t *cmdtp, int flag, int argc,
> + char * const argv[])
> +{
> + const char *name;
> + size_t bytes;
> + size_t bytes_read;
> + void *buffer;
> +
> + if (!avb_ops) {
> + printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + if (argc != 3)
> + return CMD_RET_USAGE;
> +
> + name = argv[1];
> + bytes = simple_strtoul(argv[2], NULL, 10);
This parses without error check and would for instance parse 123hello as
123.
> + buffer = malloc(bytes);
> + if (!buffer)
> + return CMD_RET_FAILURE;
> +
> + printf("Reading persistent value, name = %s, bytes = %ld\n",
> + name, bytes);
> + if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
> + &bytes_read) == AVB_IO_RESULT_OK) {
> + printf("Read %ld bytes, value = %s\n", bytes_read,
> + (char *)buffer);
> + free(buffer);
> + return CMD_RET_SUCCESS;
> + }
> +
> + printf("Failed to write in partition\n");
read
> +
> + free(buffer);
> +
> + return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_write_pvalue(cmd_tbl_t *cmdtp, int flag, int argc,
> + char * const argv[])
> +{
> + const char *name;
> + const char *value;
> +
> + if (!avb_ops) {
> + printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + if (argc != 3)
> + return CMD_RET_USAGE;
> +
> + name = argv[1];
> + value = argv[2];
> +
> + printf("Writing persistent value, name = %s, value = %s\n",
> + name, value);
> + if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
> + (const uint8_t *)value) ==
> + AVB_IO_RESULT_OK) {
> + printf("Wrote %ld bytes\n", strlen(value) + 1);
> + return CMD_RET_SUCCESS;
> + }
> +
> + printf("Failed to write persistent value\n");
> +
> + return CMD_RET_FAILURE;
> +}
> +
> static cmd_tbl_t cmd_avb[] = {
> U_BOOT_CMD_MKENT(init, 2, 0, do_avb_init, "", ""),
> U_BOOT_CMD_MKENT(read_rb, 2, 0, do_avb_read_rb, "", ""),
> @@ -350,6 +420,10 @@ static cmd_tbl_t cmd_avb[] = {
> U_BOOT_CMD_MKENT(read_part_hex, 4, 0, do_avb_read_part_hex, "", ""),
> U_BOOT_CMD_MKENT(write_part, 5, 0, do_avb_write_part, "", ""),
> U_BOOT_CMD_MKENT(verify, 1, 0, do_avb_verify_part, "", ""),
> +#ifdef CONFIG_OPTEE_TA_AVB
> + U_BOOT_CMD_MKENT(read_pvalue, 3, 0, do_avb_read_pvalue, "", ""),
> + U_BOOT_CMD_MKENT(write_pvalue, 3, 0, do_avb_write_pvalue, "", ""),
> +#endif
> };
>
> static int do_avb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> @@ -384,6 +458,10 @@ U_BOOT_CMD(
> " partition <partname> and print to stdout\n"
> "avb write_part <partname> <offset> <num> <addr> - write <num> bytes to\n"
> " <partname> by <offset> using data from <addr>\n"
> +#ifdef CONFIG_OPTEE_TA_AVB
> + "avb read_pvalue <name> <bytes> - read a persistent value <name>\n"
> + "avb write_pvalue <name> <value> - write a persistent value <name>\n"
> +#endif
> "avb verify - run verification process using hash data\n"
> " from vbmeta structure\n"
> );
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index a8c5a3e..292ad8f 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -647,6 +647,10 @@ static AvbIOResult invoke_func(struct AvbOpsData *ops_data, u32 func,
> return AVB_IO_RESULT_OK;
> case TEE_ERROR_OUT_OF_MEMORY:
> return AVB_IO_RESULT_ERROR_OOM;
> + case TEE_ERROR_STORAGE_NO_SPACE:
> + return AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE;
> + case TEE_ERROR_ITEM_NOT_FOUND:
> + return AVB_IO_RESULT_ERROR_NO_SUCH_VALUE;
> case TEE_ERROR_TARGET_DEAD:
> /*
> * The TA has paniced, close the session to reload the TA
> @@ -847,6 +851,120 @@ static AvbIOResult get_size_of_partition(AvbOps *ops,
> return AVB_IO_RESULT_OK;
> }
>
> +static AvbIOResult read_persistent_value(AvbOps *ops,
> + const char *name,
> + size_t buffer_size,
> + u8 *out_buffer,
> + size_t *out_num_bytes_read)
> +{
> + AvbIOResult rc;
> + struct tee_shm *shm_name;
> + struct tee_shm *shm_buf;
> + struct tee_param param[2];
> + struct udevice *tee;
> +
> + if (get_open_session(ops->user_data))
> + return AVB_IO_RESULT_ERROR_IO;
> +
> + tee = ((struct AvbOpsData *)ops->user_data)->tee;
> +
> + rc = tee_shm_alloc(tee, strlen(name) + 1,
> + TEE_SHM_ALLOC, &shm_name);
I'd prefer using a helper variable to hold the value of strlen(name) + 1
since it's used a few times in this function.
> + if (rc)
> + return AVB_IO_RESULT_ERROR_OOM;
> +
> + rc = tee_shm_alloc(tee, buffer_size,
> + TEE_SHM_ALLOC, &shm_buf);
> + if (rc) {
> + rc = AVB_IO_RESULT_ERROR_OOM;
> + goto free_name;
> + }
> +
> + memcpy(shm_name->addr, name, strlen(name) + 1);
> +
> + memset(param, 0, sizeof(param));
> + param[0].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INPUT;
> + param[0].u.memref.shm = shm_name;
> + param[0].u.memref.size = strlen(name) + 1;
> + param[1].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
> + param[1].u.memref.shm = shm_buf;
> + param[1].u.memref.size = buffer_size;
> +
> + rc = invoke_func(ops->user_data, TA_AVB_CMD_READ_PERSIST_VALUE,
> + 2, param);
> + if (rc)
> + goto out;
> +
> + *out_num_bytes_read = param[1].u.memref.size;
checking that param[1].u.memref.size isn't larger than buffer_size seems
like a good idea, just in case.
> +
> + memcpy(out_buffer, shm_buf->addr, *out_num_bytes_read);
> +
> + return AVB_IO_RESULT_OK;
Now you're leaking shm_buf and shm_name
> +
> +out:
> + tee_shm_free(shm_buf);
> +free_name:
> + tee_shm_free(shm_name);
> +
> + return rc;
> +}
> +
> +static AvbIOResult write_persistent_value(AvbOps *ops,
> + const char *name,
> + size_t value_size,
> + const u8 *value)
> +{
> + AvbIOResult rc;
> + struct tee_shm *shm_name;
> + struct tee_shm *shm_buf;
> + struct tee_param param[2];
> + struct udevice *tee;
> +
> + if (get_open_session(ops->user_data))
> + return AVB_IO_RESULT_ERROR_IO;
> +
> + tee = ((struct AvbOpsData *)ops->user_data)->tee;
> +
> + if (!value_size)
> + return AVB_IO_RESULT_ERROR_NO_SUCH_VALUE;
> +
> + rc = tee_shm_alloc(tee, strlen(name) + 1,
> + TEE_SHM_ALLOC, &shm_name);
> + if (rc)
> + return AVB_IO_RESULT_ERROR_OOM;
> +
> + rc = tee_shm_alloc(tee, value_size,
> + TEE_SHM_ALLOC, &shm_buf);
> + if (rc) {
> + rc = AVB_IO_RESULT_ERROR_OOM;
> + goto free_name;
> + }
> +
> + memcpy(shm_name->addr, name, strlen(name) + 1);
> + memcpy(shm_buf->addr, value, value_size);
> +
> + memset(param, 0, sizeof(param));
> + param[0].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INPUT;
> + param[0].u.memref.shm = shm_name;
> + param[0].u.memref.size = strlen(name) + 1;
> + param[1].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INPUT;
> + param[1].u.memref.shm = shm_buf;
> + param[1].u.memref.size = value_size;
> +
> + rc = invoke_func(ops->user_data, TA_AVB_CMD_WRITE_PERSIST_VALUE,
> + 2, param);
> + if (rc)
> + goto out;
> +
> + return AVB_IO_RESULT_OK;
Now you're leaking shm_buf and shm_name
> +
> +out:
> + tee_shm_free(shm_buf);
> +free_name:
> + tee_shm_free(shm_name);
> +
> + return rc;
> +}
> /**
> * ============================================================================
> * AVB2.0 AvbOps alloc/initialisation/free
> @@ -870,6 +988,10 @@ AvbOps *avb_ops_alloc(int boot_device)
> ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
> ops_data->ops.get_unique_guid_for_partition =
> get_unique_guid_for_partition;
> +#ifdef CONFIG_OPTEE_TA_AVB
> + ops_data->ops.write_persistent_value = write_persistent_value;
> + 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;
>
> diff --git a/include/tee.h b/include/tee.h
> index 98b1c9c..69de924 100644
> --- a/include/tee.h
> +++ b/include/tee.h
> @@ -42,7 +42,9 @@
> #define TEE_ERROR_COMMUNICATION 0xffff000e
> #define TEE_ERROR_SECURITY 0xffff000f
> #define TEE_ERROR_OUT_OF_MEMORY 0xffff000c
> +#define TEE_ERROR_OVERFLOW 0xffff300f
> #define TEE_ERROR_TARGET_DEAD 0xffff3024
> +#define TEE_ERROR_STORAGE_NO_SPACE 0xffff3041
>
> #define TEE_ORIGIN_COMMS 0x00000002
> #define TEE_ORIGIN_TEE 0x00000003
> diff --git a/include/tee/optee_ta_avb.h b/include/tee/optee_ta_avb.h
> index 074386a..ef84488 100644
> --- a/include/tee/optee_ta_avb.h
> +++ b/include/tee/optee_ta_avb.h
> @@ -45,4 +45,20 @@
> */
> #define TA_AVB_CMD_WRITE_LOCK_STATE 3
>
> +/*
> + * Reads a persistent value corresponding to the given name.
> + *
> + * in params[0].u.memref: persistent value name
> + * ioout params[1].u.memref: read persistent value buffer
This is out only, even if the size of the buffer is supplied it's still
considered an out parameter.
Cheers,
Jens
> + */
> +#define TA_AVB_CMD_READ_PERSIST_VALUE 4
> +
> +/*
> + * Writes a persistent value corresponding to the given name.
> + *
> + * in params[0].u.memref: persistent value name
> + * in params[1].u.memref: persistent value buffer to write
> + */
> +#define TA_AVB_CMD_WRITE_PERSIST_VALUE 5
> +
> #endif /* __TA_AVB_H */
> --
> 2.7.4
>
More information about the U-Boot
mailing list