Re: [U-Boot] [PATCH v2 1/1] avb: add support for named persistent values

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;
  

  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