Reviewed-by: Sam Protsenko <semen.protse...@linaro.org>
On Thu, Feb 14, 2019 at 2:37 PM Igor Opaniuk <igor.opan...@linaro.org> wrote: > > AVB 2.0 spec. revision 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 > > Reviewed-by: Simon Glass <s...@chromium.org> > Signed-off-by: Igor Opaniuk <igor.opan...@linaro.org> > --- > > v5: > - fix failing test on sandbox_flattree target > - keep all defined globally variables in a driver-private data struct, which > permits to probe multiple tee sandbox devices > > v4: > - extend tee sandbox tee driver to support persistent values > - fix/re-test avb_persistent test on sandbox configuration > > v3: > - fix possible mem lick in avb_read_persistent/avb_write_persistent > - add additional sanity checks > - cover avb read_pvalue/write_pvalue commands with python tests > > 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 | 125 > +++++++++++++++++++++++++++++++++++++++++++++ > drivers/tee/sandbox.c | 121 ++++++++++++++++++++++++++++++++++++------- > include/sandboxtee.h | 15 ++++-- > include/tee.h | 2 + > include/tee/optee_ta_avb.h | 16 ++++++ > test/py/tests/test_avb.py | 16 ++++++ > 7 files changed, 351 insertions(+), 22 deletions(-) > > diff --git a/cmd/avb.c b/cmd/avb.c > index ff00be4..c5af4a2 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; > + char *endp; > + > + 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], &endp, 10); > + if (*endp && *endp != '\n') > + return CMD_RET_USAGE; > + > + buffer = malloc(bytes); > + if (!buffer) > + return CMD_RET_FAILURE; > + > + 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 read persistent value\n"); > + > + 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]; > + > + 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..32034d9 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,123 @@ 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; > + size_t name_size = strlen(name) + 1; > + > + 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, name_size, > + TEE_SHM_ALLOC, &shm_name); > + 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, name_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 = name_size; > + 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; > + > + if (param[1].u.memref.size > buffer_size) { > + rc = AVB_IO_RESULT_ERROR_NO_SUCH_VALUE; > + goto out; > + } > + > + *out_num_bytes_read = param[1].u.memref.size; > + > + memcpy(out_buffer, shm_buf->addr, *out_num_bytes_read); > + > +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; > + size_t name_size = strlen(name) + 1; > + > + 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, name_size, > + 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, name_size); > + 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 = name_size; > + 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; > + > +out: > + tee_shm_free(shm_buf); > +free_name: > + tee_shm_free(shm_name); > + > + return rc; > +} > /** > * > ============================================================================ > * AVB2.0 AvbOps alloc/initialisation/free > @@ -870,6 +991,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/drivers/tee/sandbox.c b/drivers/tee/sandbox.c > index ccddb03..a136bc9 100644 > --- a/drivers/tee/sandbox.c > +++ b/drivers/tee/sandbox.c > @@ -14,6 +14,7 @@ > * available. > */ > > +static const u32 pstorage_max = 16; > /** > * struct ta_entry - TA entries > * @uuid: UUID of an emulated TA > @@ -24,8 +25,11 @@ > */ > struct ta_entry { > struct tee_optee_ta_uuid uuid; > - u32 (*open_session)(uint num_params, struct tee_param *params); > - u32 (*invoke_func)(u32 func, uint num_params, struct tee_param > *params); > + u32 (*open_session)(struct udevice *dev, uint num_params, > + struct tee_param *params); > + u32 (*invoke_func)(struct udevice *dev, > + u32 func, uint num_params, > + struct tee_param *params); > }; > > #ifdef CONFIG_OPTEE_TA_AVB > @@ -59,10 +63,8 @@ bad_params: > return TEE_ERROR_BAD_PARAMETERS; > } > > -static u64 ta_avb_rollback_indexes[TA_AVB_MAX_ROLLBACK_LOCATIONS]; > -static u32 ta_avb_lock_state; > - > -static u32 ta_avb_open_session(uint num_params, struct tee_param *params) > +static u32 ta_avb_open_session(struct udevice *dev, uint num_params, > + struct tee_param *params) > { > /* > * We don't expect additional parameters when opening a session to > @@ -73,12 +75,17 @@ static u32 ta_avb_open_session(uint num_params, struct > tee_param *params) > num_params, params); > } > > -static u32 ta_avb_invoke_func(u32 func, uint num_params, > +static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params, > struct tee_param *params) > { > + struct sandbox_tee_state *state = dev_get_priv(dev); > + ENTRY e, *ep; > + char *name; > u32 res; > uint slot; > u64 val; > + char *value; > + u32 value_sz; > > switch (func) { > case TA_AVB_CMD_READ_ROLLBACK_INDEX: > @@ -91,12 +98,12 @@ static u32 ta_avb_invoke_func(u32 func, uint num_params, > return res; > > slot = params[0].u.value.a; > - if (slot >= ARRAY_SIZE(ta_avb_rollback_indexes)) { > + if (slot >= ARRAY_SIZE(state->ta_avb_rollback_indexes)) { > printf("Rollback index slot out of bounds %u\n", > slot); > return TEE_ERROR_BAD_PARAMETERS; > } > > - val = ta_avb_rollback_indexes[slot]; > + val = state->ta_avb_rollback_indexes[slot]; > params[1].u.value.a = val >> 32; > params[1].u.value.b = val; > return TEE_SUCCESS; > @@ -111,16 +118,16 @@ static u32 ta_avb_invoke_func(u32 func, uint num_params, > return res; > > slot = params[0].u.value.a; > - if (slot >= ARRAY_SIZE(ta_avb_rollback_indexes)) { > + if (slot >= ARRAY_SIZE(state->ta_avb_rollback_indexes)) { > printf("Rollback index slot out of bounds %u\n", > slot); > return TEE_ERROR_BAD_PARAMETERS; > } > > val = (u64)params[1].u.value.a << 32 | params[1].u.value.b; > - if (val < ta_avb_rollback_indexes[slot]) > + if (val < state->ta_avb_rollback_indexes[slot]) > return TEE_ERROR_SECURITY; > > - ta_avb_rollback_indexes[slot] = val; > + state->ta_avb_rollback_indexes[slot] = val; > return TEE_SUCCESS; > > case TA_AVB_CMD_READ_LOCK_STATE: > @@ -132,7 +139,7 @@ static u32 ta_avb_invoke_func(u32 func, uint num_params, > if (res) > return res; > > - params[0].u.value.a = ta_avb_lock_state; > + params[0].u.value.a = state->ta_avb_lock_state; > return TEE_SUCCESS; > > case TA_AVB_CMD_WRITE_LOCK_STATE: > @@ -144,13 +151,64 @@ static u32 ta_avb_invoke_func(u32 func, uint num_params, > if (res) > return res; > > - if (ta_avb_lock_state != params[0].u.value.a) { > - ta_avb_lock_state = params[0].u.value.a; > - memset(ta_avb_rollback_indexes, 0, > - sizeof(ta_avb_rollback_indexes)); > + if (state->ta_avb_lock_state != params[0].u.value.a) { > + state->ta_avb_lock_state = params[0].u.value.a; > + memset(state->ta_avb_rollback_indexes, 0, > + sizeof(state->ta_avb_rollback_indexes)); > } > > return TEE_SUCCESS; > + case TA_AVB_CMD_READ_PERSIST_VALUE: > + res = check_params(TEE_PARAM_ATTR_TYPE_MEMREF_INPUT, > + TEE_PARAM_ATTR_TYPE_MEMREF_INOUT, > + TEE_PARAM_ATTR_TYPE_NONE, > + TEE_PARAM_ATTR_TYPE_NONE, > + num_params, params); > + if (res) > + return res; > + > + name = params[0].u.memref.shm->addr; > + > + value = params[1].u.memref.shm->addr; > + value_sz = params[1].u.memref.size; > + > + e.key = name; > + e.data = NULL; > + hsearch_r(e, FIND, &ep, &state->pstorage_htab, 0); > + if (!ep) > + return TEE_ERROR_ITEM_NOT_FOUND; > + > + value_sz = strlen(ep->data); > + memcpy(value, ep->data, value_sz); > + > + return TEE_SUCCESS; > + case TA_AVB_CMD_WRITE_PERSIST_VALUE: > + res = check_params(TEE_PARAM_ATTR_TYPE_MEMREF_INPUT, > + TEE_PARAM_ATTR_TYPE_MEMREF_INPUT, > + TEE_PARAM_ATTR_TYPE_NONE, > + TEE_PARAM_ATTR_TYPE_NONE, > + num_params, params); > + if (res) > + return res; > + > + name = params[0].u.memref.shm->addr; > + > + value = params[1].u.memref.shm->addr; > + value_sz = params[1].u.memref.size; > + > + e.key = name; > + e.data = NULL; > + hsearch_r(e, FIND, &ep, &state->pstorage_htab, 0); > + if (ep) > + hdelete_r(e.key, &state->pstorage_htab, 0); > + > + e.key = name; > + e.data = value; > + hsearch_r(e, ENTER, &ep, &state->pstorage_htab, 0); > + if (!ep) > + return TEE_ERROR_OUT_OF_MEMORY; > + > + return TEE_SUCCESS; > > default: > return TEE_ERROR_NOT_SUPPORTED; > @@ -225,7 +283,7 @@ static int sandbox_tee_open_session(struct udevice *dev, > return 0; > } > > - arg->ret = ta->open_session(num_params, params); > + arg->ret = ta->open_session(dev, num_params, params); > arg->ret_origin = TEE_ORIGIN_TRUSTED_APP; > > if (!arg->ret) { > @@ -261,7 +319,7 @@ static int sandbox_tee_invoke_func(struct udevice *dev, > return -EINVAL; > } > > - arg->ret = ta->invoke_func(arg->func, num_params, params); > + arg->ret = ta->invoke_func(dev, arg->func, num_params, params); > arg->ret_origin = TEE_ORIGIN_TRUSTED_APP; > > return 0; > @@ -285,6 +343,29 @@ static int sandbox_tee_shm_unregister(struct udevice > *dev, struct tee_shm *shm) > return 0; > } > > +static int sandbox_tee_remove(struct udevice *dev) > +{ > + struct sandbox_tee_state *state = dev_get_priv(dev); > + > + hdestroy_r(&state->pstorage_htab); > + > + return 0; > +} > + > +static int sandbox_tee_probe(struct udevice *dev) > +{ > + struct sandbox_tee_state *state = dev_get_priv(dev); > + /* > + * With this hastable we emulate persistent storage, > + * which should contain persistent values > + * between different sessions/command invocations. > + */ > + if (!hcreate_r(pstorage_max, &state->pstorage_htab)) > + return TEE_ERROR_OUT_OF_MEMORY; > + > + return 0; > +} > + > static const struct tee_driver_ops sandbox_tee_ops = { > .get_version = sandbox_tee_get_version, > .open_session = sandbox_tee_open_session, > @@ -305,4 +386,6 @@ U_BOOT_DRIVER(sandbox_tee) = { > .of_match = sandbox_tee_match, > .ops = &sandbox_tee_ops, > .priv_auto_alloc_size = sizeof(struct sandbox_tee_state), > + .probe = sandbox_tee_probe, > + .remove = sandbox_tee_remove, > }; > diff --git a/include/sandboxtee.h b/include/sandboxtee.h > index 44f653d..419643a 100644 > --- a/include/sandboxtee.h > +++ b/include/sandboxtee.h > @@ -6,16 +6,25 @@ > #ifndef __SANDBOXTEE_H > #define __SANDBOXTEE_H > > +#include <search.h> > +#include <tee/optee_ta_avb.h> > + > /** > * struct sandbox_tee_state - internal state of the sandbox TEE > - * @session: current open session > - * @num_shms: number of registered shared memory objects > - * @ta: Trusted Application of current session > + * @session: current open session > + * @num_shms: number of registered shared memory objects > + * @ta: Trusted Application of current session > + * @ta_avb_rollback_indexes TA avb rollback indexes storage > + * @ta_avb_lock_state TA avb lock state storage > + * @pstorage_htab named persistent values storage > */ > struct sandbox_tee_state { > u32 session; > int num_shms; > void *ta; > + u64 ta_avb_rollback_indexes[TA_AVB_MAX_ROLLBACK_LOCATIONS]; > + u32 ta_avb_lock_state; > + struct hsearch_data pstorage_htab; > }; > > #endif /*__SANDBOXTEE_H*/ > diff --git a/include/tee.h b/include/tee.h > index edd9f9b..02bcd9e 100644 > --- a/include/tee.h > +++ b/include/tee.h > @@ -43,7 +43,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..949875a 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 > + * out params[1].u.memref: read persistent value buffer > + */ > +#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 */ > diff --git a/test/py/tests/test_avb.py b/test/py/tests/test_avb.py > index e70a010..2bb75ed 100644 > --- a/test/py/tests/test_avb.py > +++ b/test/py/tests/test_avb.py > @@ -116,3 +116,19 @@ def test_avb_mmc_read(u_boot_console): > response = u_boot_console.run_command('cmp 0x%x 0x%x 40' % > (temp_addr, temp_addr2)) > assert response.find('64 word') > + > + > +@pytest.mark.buildconfigspec('cmd_avb') > +@pytest.mark.buildconfigspec('optee_ta_avb') > +def test_avb_persistent_values(u_boot_console): > + """Test reading/writing persistent storage to avb > + """ > + > + response = u_boot_console.run_command('avb init %s' % str(mmc_dev)) > + assert response == '' > + > + response = u_boot_console.run_command('avb write_pvalue test > value_value') > + assert response == 'Wrote 12 bytes' > + > + response = u_boot_console.run_command('avb read_pvalue test 12') > + assert response == 'Read 12 bytes, value = value_value' > -- > 2.7.4 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
