[v2,1/1] avb: add support for named persistent values

Message ID 1544809503-30893-1-git-send-email-igor.opaniuk@linaro.org
State New
Headers show
Series
  • [v2,1/1] avb: add support for named persistent values
Related show

Commit Message

Igor Opaniuk Dec. 14, 2018, 5:45 p.m.
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(+)

Comments

Igor Opaniuk Dec. 21, 2018, 8:47 p.m. | #1
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].
>
> [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 */
> --
> 2.7.4
>
Simon Glass Dec. 22, 2018, 8:52 p.m. | #2
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].
>
> [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(+)

Doesn't this need an update to the Android update test?

Regards,
Simon
Igor Opaniuk Dec. 27, 2018, 2:49 p.m. | #3
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].
> >
> > [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(+)
>
> Doesn't this need an update to the Android update test?
>
> Regards,
> Simon



--
Regards,
Igor Opaniuk
Simon Glass Dec. 27, 2018, 3:12 p.m. | #4
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
Igor Opaniuk Dec. 27, 2018, 3:21 p.m. | #5
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
Jens Wiklander Jan. 14, 2019, 9:26 a.m. | #6
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@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
>

Patch

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 */