[PATCH 0/3] avb: start handling too small buffer for reading persistent values

[PATCH 0/3] avb: start handling too small buffer for reading persistent values

[Rasmus Villemoes]

The avb_ops.h header clearly indicates that it should be possible for
the callers of the read_persistent_value interface to know that they
provided too small a buffer, and the interface should also tell the
callers how big a buffer would need to be.

However, that was never really implemented, neither here nor on the
op-tee side. So if a too small buffer is passed, the caller simply
gets a silently truncated result.

I've proposed a fix on the op-tee side:
https://github.com/OP-TEE/optee_os/pull/7959 . Since that is an ABI
change (returning an error where it previously "succeeded", for some
definition of succeed), the maintainer would like to see client side
updates as well before that can be merged. So here are some of the
changes that would be needed in U-Boot.

I have not yet looked at any of the callers of the
->read_persistent_value method via the avb_ops structure, only the
implementation of that method itself.

Rasmus Villemoes (3):
  cmd: optee_rpmb: sanitize TEE_ERROR -> E* translations
  cmd: optee_rpmb: try to allocate large enough buffer when reading
    persistent value
  avb_verify: read_persistent_value: honour contract regarding too small
    buffer

 cmd/optee_rpmb.c    | 45 ++++++++++++++++++++++++++++++---------------
 common/avb_verify.c |  5 ++++-
 2 files changed, 34 insertions(+), 16 deletions(-)

-- 
2.55.0

[PATCH 1/3] cmd: optee_rpmb: sanitize TEE_ERROR -> E* translations

[Rasmus Villemoes]

Do not translate both OUT_OF_MEMORY and STORAGE_NO_SPACE to -ENOSPC;
we have -ENOMEM which is more fitting for the former.

Similarly, -ENOENT is a better match for ITEM_NOT_FOUND than "some IO
went wrong".

None of the callers currently care about the actual error code, so
this makes no functional change, but we will add special handling of
the -ENOSPC in a later patch.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 cmd/optee_rpmb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
index b155278ee2a..cc384c7041e 100644
--- a/cmd/optee_rpmb.c
+++ b/cmd/optee_rpmb.c
@@ -52,10 +52,11 @@ static int invoke_func(u32 func, ulong num_param, struct tee_param *param)
 	case TEE_SUCCESS:
 		return 0;
 	case TEE_ERROR_OUT_OF_MEMORY:
+		return -ENOMEM;
 	case TEE_ERROR_STORAGE_NO_SPACE:
 		return -ENOSPC;
 	case TEE_ERROR_ITEM_NOT_FOUND:
-		return -EIO;
+		return -ENOENT;
 	case TEE_ERROR_TARGET_DEAD:
 		/*
 		 * The TA has paniced, close the session to reload the TA
-- 
2.55.0

[PATCH 2/3] cmd: optee_rpmb: try to allocate large enough buffer when reading persistent value

[Rasmus Villemoes]

It is implied by the comments in avb_ops.h and the translation of
TEE_ERROR_STORAGE_NO_SPACE to AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE
done in common/avb_verify.c:invoke_func() that the
TA_AVB_CMD_READ_PERSIST_VALUE could return TEE_ERROR_STORAGE_NO_SPACE
when the value is longer than the passed buffer size, and that
param[1].u.memref.size would be set to the actual size, so that one
can allocate an appropriate buffer and re-read.

However, that has AFAICT never been the case; there is no mention of
TEE_ERROR_STORAGE_NO_SPACE in the history of ta/avb/ in
https://github.com/OP-TEE/optee_os.git, and what the code does instead
is to return a value truncated to the given buffer size. In other
words, not only can one not determine the correct buffer size to
allocate, one is not even told that truncation happened.

A fix is proposed on the op-tee
side (https://github.com/OP-TEE/optee_os/pull/7959), but the
maintainer would like to see at least some callers updated before it
can get merged, which makes sense.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 cmd/optee_rpmb.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
index cc384c7041e..ba10c1d7b82 100644
--- a/cmd/optee_rpmb.c
+++ b/cmd/optee_rpmb.c
@@ -71,8 +71,8 @@ static int invoke_func(u32 func, ulong num_param, struct tee_param *param)
 }
 
 static int read_persistent_value(const char *name,
-				 size_t buffer_size,
-				 u8 *out_buffer,
+				 size_t size_hint,
+				 char **out_buffer,
 				 size_t *out_num_bytes_read)
 {
 	int rc = 0;
@@ -80,6 +80,8 @@ static int read_persistent_value(const char *name,
 	struct tee_shm *shm_buf;
 	struct tee_param param[2];
 	size_t name_size = strlen(name) + 1;
+	size_t buffer_size = size_hint;
+	int retry = 1;
 
 	if (!tee)
 		if (avb_ta_open_session())
@@ -92,6 +94,7 @@ static int read_persistent_value(const char *name,
 		goto close_session;
 	}
 
+again:
 	rc = tee_shm_alloc(tee, buffer_size,
 			   TEE_SHM_ALLOC, &shm_buf);
 	if (rc) {
@@ -111,6 +114,14 @@ static int read_persistent_value(const char *name,
 
 	rc = invoke_func(TA_AVB_CMD_READ_PERSIST_VALUE,
 			 2, param);
+
+	if (rc == -ENOSPC && param[1].u.memref.size > buffer_size && retry) {
+		retry = 0;
+		tee_shm_free(shm_buf);
+		buffer_size = param[1].u.memref.size;
+		goto again;
+	}
+
 	if (rc)
 		goto out;
 
@@ -120,8 +131,9 @@ static int read_persistent_value(const char *name,
 	}
 
 	*out_num_bytes_read = param[1].u.memref.size;
-
-	memcpy(out_buffer, shm_buf->addr, *out_num_bytes_read);
+	*out_buffer = memdup(shm_buf->addr, *out_num_bytes_read);
+	if (!*out_buffer)
+		rc = -ENOMEM;
 
 out:
 	tee_shm_free(shm_buf);
@@ -199,22 +211,24 @@ int do_optee_rpmb_read(struct cmd_tbl *cmdtp, int flag, int argc,
 	const char *name;
 	size_t bytes;
 	size_t bytes_read;
-	void *buffer;
+	char *buffer = NULL;
 	char *endp;
 
-	if (argc != 3)
+	/* Use a third argument merely as a size hint. */
+	if (argc < 2 || argc > 3)
 		return CMD_RET_USAGE;
 
 	name = argv[1];
-	bytes = dectoul(argv[2], &endp);
-	if (*endp && *endp != '\n')
-		return CMD_RET_USAGE;
-
-	buffer = malloc(bytes);
-	if (!buffer)
-		return CMD_RET_FAILURE;
+	if (argc >= 3) {
+		bytes = dectoul(argv[2], &endp);
+		if (*endp && *endp != '\n')
+			return CMD_RET_USAGE;
+	} else {
+		/* Probably enough for most cases to not require two roundtrips. */
+		bytes = 64;
+	}
 
-	if (read_persistent_value(name, bytes, buffer, &bytes_read) == 0) {
+	if (read_persistent_value(name, bytes, &buffer, &bytes_read) == 0) {
 		printf("Read %zu bytes, value = %s\n", bytes_read,
 		       (char *)buffer);
 		free(buffer);
-- 
2.55.0

[PATCH 3/3] avb_verify: read_persistent_value: honour contract regarding too small buffer

[Rasmus Villemoes]

The avb_ops.h header file, which has been imported from the upstream
libavb, says:

   /* Reads a persistent value corresponding to the given |name|. The value is
    * returned in |out_buffer| which must point to |buffer_size| bytes. On
    * success |out_num_bytes_read| contains the number of bytes read into
    * |out_buffer|. If AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is returned,
    * |out_num_bytes_read| contains the number of bytes that would have been read
    * which can be used to allocate a buffer.

The invoke_func() wrapper does translate a TEE_ERROR_STORAGE_NO_SPACE
return to AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE. However, this
implementation of the read_persistent_value method currently never
updates *out_num_bytes_read in case of an error.

Also note that currently, at least the upstream optee-os
implementation of TA_AVB_CMD_READ_PERSIST_VALUE never returns
TEE_ERROR_STORAGE_NO_SPACE, but instead silently returns a truncated
value, with no way for the caller to know that happened. A fix has
been proposed (https://github.com/OP-TEE/optee_os/pull/7959).

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 common/avb_verify.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 76c523fd0ba..af166e7dfba 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -963,8 +963,11 @@ static AvbIOResult read_persistent_value(AvbOps *ops,
 
 	rc = invoke_func(ops->user_data, TA_AVB_CMD_READ_PERSIST_VALUE,
 			 2, param);
-	if (rc)
+	if (rc) {
+		if (rc == AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE)
+			*out_num_bytes_read = param[1].u.memref.size;
 		goto out;
+	}
 
 	if (param[1].u.memref.size > buffer_size) {
 		rc = AVB_IO_RESULT_ERROR_NO_SUCH_VALUE;
-- 
2.55.0

Re: [PATCH 1/3] cmd: optee_rpmb: sanitize TEE_ERROR -> E* translations

[Mattijs Korpershoek]

Hi Rasmus,

Thank you for the patch.

On Fri, Aug 28, 2026 at 12:50, Rasmus Villemoes <ravi@prevas.dk> wrote:

> Do not translate both OUT_OF_MEMORY and STORAGE_NO_SPACE to -ENOSPC;
> we have -ENOMEM which is more fitting for the former.
>
> Similarly, -ENOENT is a better match for ITEM_NOT_FOUND than "some IO
> went wrong".
>
> None of the callers currently care about the actual error code, so
> this makes no functional change, but we will add special handling of
> the -ENOSPC in a later patch.
>
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  cmd/optee_rpmb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Re: [PATCH 1/3] cmd: optee_rpmb: sanitize TEE_ERROR -> E* translations

[Rasmus Villemoes]

On Wed, Sep 02 2026, "Mattijs Korpershoek" <mkorpershoek@kernel.org> wrote:

> Hi Rasmus,
>
> Thank you for the patch.
>
> On Fri, Aug 28, 2026 at 12:50, Rasmus Villemoes <ravi@prevas.dk> wrote:
>
>> Do not translate both OUT_OF_MEMORY and STORAGE_NO_SPACE to -ENOSPC;
>> we have -ENOMEM which is more fitting for the former.
>>
>> Similarly, -ENOENT is a better match for ITEM_NOT_FOUND than "some IO
>> went wrong".
>>
>> None of the callers currently care about the actual error code, so
>> this makes no functional change, but we will add special handling of
>> the -ENOSPC in a later patch.
>>
>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

Thanks.

Note that the patch on the optee side is still in flux, and I have an
updated version of the U-Boot patches locally. None of this should be
merged until the optee side is sorted out and merged.

Rasmus

Re: [PATCH 2/3] cmd: optee_rpmb: try to allocate large enough buffer when reading persistent value

[Mattijs Korpershoek]

Hi Rasmus,

Thank you for the patch.

On Fri, Aug 28, 2026 at 12:50, Rasmus Villemoes <ravi@prevas.dk> wrote:

> It is implied by the comments in avb_ops.h and the translation of
> TEE_ERROR_STORAGE_NO_SPACE to AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE
> done in common/avb_verify.c:invoke_func() that the
> TA_AVB_CMD_READ_PERSIST_VALUE could return TEE_ERROR_STORAGE_NO_SPACE
> when the value is longer than the passed buffer size, and that
> param[1].u.memref.size would be set to the actual size, so that one
> can allocate an appropriate buffer and re-read.
>
> However, that has AFAICT never been the case; there is no mention of
> TEE_ERROR_STORAGE_NO_SPACE in the history of ta/avb/ in
> https://github.com/OP-TEE/optee_os.git, and what the code does instead
> is to return a value truncated to the given buffer size. In other
> words, not only can one not determine the correct buffer size to
> allocate, one is not even told that truncation happened.
>
> A fix is proposed on the op-tee
> side (https://github.com/OP-TEE/optee_os/pull/7959), but the
> maintainer would like to see at least some callers updated before it
> can get merged, which makes sense.
>
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

Re: [PATCH 3/3] avb_verify: read_persistent_value: honour contract regarding too small buffer

[Mattijs Korpershoek]

Hi Rasmus,

Thank you for the patch.

On Fri, Aug 28, 2026 at 12:50, Rasmus Villemoes <ravi@prevas.dk> wrote:

> The avb_ops.h header file, which has been imported from the upstream
> libavb, says:
>
>    /* Reads a persistent value corresponding to the given |name|. The value is
>     * returned in |out_buffer| which must point to |buffer_size| bytes. On
>     * success |out_num_bytes_read| contains the number of bytes read into
>     * |out_buffer|. If AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is returned,
>     * |out_num_bytes_read| contains the number of bytes that would have been read
>     * which can be used to allocate a buffer.
>
> The invoke_func() wrapper does translate a TEE_ERROR_STORAGE_NO_SPACE
> return to AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE. However, this
> implementation of the read_persistent_value method currently never
> updates *out_num_bytes_read in case of an error.
>
> Also note that currently, at least the upstream optee-os
> implementation of TA_AVB_CMD_READ_PERSIST_VALUE never returns
> TEE_ERROR_STORAGE_NO_SPACE, but instead silently returns a truncated
> value, with no way for the caller to know that happened. A fix has
> been proposed (https://github.com/OP-TEE/optee_os/pull/7959).
>
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

Ilias, this series touches both AVB and TEE. Can you pick up all 3
patches or do you want me to do so?

If you wish that I pick them up, please ack them first.

Thanks
Mattijs