[U-Boot] [PATCH v2 1/1] avb: add support for named persistent values
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@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); + 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"); + + 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); + 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; + + memcpy(out_buffer, shm_buf->addr, *out_num_bytes_read); + + return AVB_IO_RESULT_OK; + +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; + +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 + */ +#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 */
Update: Patch for OP-TEE AVB trusted application (which introduces implementation for persistent named values support on secure world side) was successfully merged [1].
[1]: https://github.com/OP-TEE/optee_os/pull/2699
On Fri, 14 Dec 2018 at 19:45, Igor Opaniuk igor.opaniuk@linaro.org 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].
Signed-off-by: Igor Opaniuk igor.opaniuk@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);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");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);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;memcpy(out_buffer, shm_buf->addr, *out_num_bytes_read);return AVB_IO_RESULT_OK;+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;+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
- */
+#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
Hi Igor,
On Fri, 14 Dec 2018 at 10:45, Igor Opaniuk igor.opaniuk@linaro.org 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].
Signed-off-by: Igor Opaniuk igor.opaniuk@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(+)
Doesn't this need an update to the Android update test?
Regards, Simon
Hi Simon,
Could you please point me to the update test you mean? (I assume it's "test_avb.py"?)
Thanks
BR, Igor
On Sat, 22 Dec 2018 at 22:52, Simon Glass sjg@chromium.org wrote:
Hi Igor,
On Fri, 14 Dec 2018 at 10:45, Igor Opaniuk igor.opaniuk@linaro.org 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].
Signed-off-by: Igor Opaniuk igor.opaniuk@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(+)
Doesn't this need an update to the Android update test?
Regards, Simon
-- Regards, Igor Opaniuk
Hi Igor,
On Thu, 27 Dec 2018 at 07:50, Igor Opaniuk igor.opaniuk@linaro.org wrote:
Hi Simon,
Could you please point me to the update test you mean? (I assume it's "test_avb.py"?)
Yes that's right. The test should cover the functionality of the feature.
Regards, Simon
ok, np. will send in v3 patch
On Thu, 27 Dec 2018 at 17:12, Simon Glass sjg@chromium.org wrote:
Hi Igor,
On Thu, 27 Dec 2018 at 07:50, Igor Opaniuk igor.opaniuk@linaro.org wrote:
Hi Simon,
Could you please point me to the update test you mean? (I assume it's "test_avb.py"?)
Yes that's right. The test should cover the functionality of the feature.
Regards, Simon
Thanks for the notice, will be added in v3!
-- Regards, Igor Opaniuk
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].
Signed-off-by: Igor Opaniuk igor.opaniuk@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;
- 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
participants (3)
-
Igor Opaniuk -
Jens Wiklander -
Simon Glass