fix open issue 4577 TEE_SetTAPersistentTime() is not persistent accro… #7506

jarvisDang · 2025-08-18T03:19:10Z

…ss reboots #4577

fix open issue 4577, Link: #4577

etienne-lms

This looks a nice step into TA persistent storage support.

The commit message should be refined, e.g.:

core: tee: persistent storage of TA time offset

Implement persistent storage of TA time offset in OP-TEE
private secure storage. TA time offset value is stored in TA
stroage space under file name "ta_time_offs".

Fixes: #4577
Signed-off-by: ...

For TEE_SetTAPersistentTime() to be atomic, I think the mutex should cover both update in RAM and in persistent storage.

etienne-lms · 2025-08-18T08:48:58Z

core/tee/tee_time_generic.c

 #include <utee_defines.h>
+#include <tee/tee_fs.h>
+#include <tee/tee_pobj.h>
+


Nitpicking: sort in alphabetical order (and by the way swap string.h/stdlib.h ordering) and remove the last empty line.

#include <kernel/mutex.h> #include <kernel/panic.h> #include <kernel/tee_time.h> #include <stdlib.h> #include <string.h> #include <tee/tee_fs.h> #include <tee/tee_pobj.h> #include <utee_defines.h>

etienne-lms · 2025-08-18T08:51:34Z

core/tee/tee_time_generic.c

+       size_t n;
+       struct tee_ta_time_offs *o;


As per OP-TEE coding style, these require a initialization value:

Suggested change

size_t n;

struct tee_ta_time_offs *o;

size_t n = 0;

struct tee_ta_time_offs *o = NULL;

etienne-lms · 2025-08-18T08:51:55Z

core/tee/tee_time_generic.c

+       size_t n;
+       struct tee_ta_time_offs *o;
+
+       mutex_lock(&tee_time_mtx);


Could you use tabulations instead of space chars for indenation?

etienne-lms · 2025-08-18T08:54:36Z

core/tee/tee_time_generic.c

+
+        TEE_Time o;
+        bool pos = false;
+        TEE_Result res = tee_time_ta_storage_read(uuid, &o, &pos);


Please define all local variables at instruction block entry (and provide them with an initialization value).

etienne-lms · 2025-08-18T08:59:38Z

core/tee/tee_time_generic.c

 static size_t tee_time_num_offs;
+static struct mutex tee_time_mtx = MUTEX_INITIALIZER;
+
+#define TA_TIME_OFFS_ID "ta_time_offs"


Nitpicking: could you define this macro at source file entry, before struct tee_ta_time_offs definition?

etienne-lms · 2025-08-18T09:06:44Z

core/tee/tee_time_generic.c

-	size_t n;
-	struct tee_ta_time_offs *o;
+       size_t idx = 0;
+       TEE_Result res = tee_time_ta_set_offs_mem(uuid, offs, positive, &idx);


Need an empty line between local variable definitions and instructions. Suggestion:

TEE_Result res = TEE_ERROR_GENERIC; size_t idx = 0; res = tee_time_ta_set_offs_mem(uuid, offs, positive, &idx);

etienne-lms · 2025-08-18T09:12:14Z

core/tee/tee_time_generic.c

+                          NULL, 0,
+                          NULL, 0,
+                          &o, NULL, sizeof(o),
+                          &fh);


Prefer:

res = fops->create(po, true, NULL, 0, NULL, 0, &o, NULL, sizeof(o), &fh);

etienne-lms · 2025-08-18T09:24:18Z

core/tee/tee_time_generic.c

+       if (!fops)
+               return TEE_ERROR_TIME_NOT_SET; /* storage unavailable: treat as not set */
+
+       res = tee_pobj_get((TEE_UUID *)uuid, TA_TIME_OFFS_ID,


Needs a (void *) cast to discard const qualifier, as reported by CI make tests.

Or perhaps even better, fix tee_pobj_get() in a separate patch.

Indeed, adding const to the few first arguments of tee_pobj_get() would help.

jarvisDang · 2025-08-18T10:36:42Z

thanks for @etienne-lms @jenswi-linaro 's detailed comments, I'm working on it.

jenswi-linaro · 2025-08-20T07:54:33Z

Please fix the checkpatch issues.

jarvisDang · 2025-08-20T07:59:20Z

Please fix the checkpatch issues.

working on it

jarvisDang · 2025-08-21T05:05:23Z

the checkpatch passed from my forked repo https://github.com/jarvisDang/optee_os/actions/runs/17115078518

jarvisDang · 2025-08-21T08:50:07Z

why the code style check issues showed in the log not match my latest source code?

jforissier · 2025-08-21T08:57:39Z

the checkpatch passed from my forked repo https://github.com/jarvisDang/optee_os/actions/runs/17115078518

In the forked repo, only one commit is checked (the topmost in the branch). Here we can see that the global diff is OK (see the "checkdiff" line), but some commits are not (see the the "checkpatch" lines). Please squash all the commits into one.

jarvisDang · 2025-08-21T09:05:58Z

@jforissier thanks for explanation, I will squash all the fixup commits and try again

jenswi-linaro · 2025-08-22T05:52:09Z

You can run checkpatch locally with, for instance, ./scripts/checkpatch.sh github/master.., depending on the base of your branch.

jarvisDang · 2025-08-24T16:35:29Z

You can run checkpatch locally with, for instance, ./scripts/checkpatch.sh github/master.., depending on the base of your branch.

glad to know that, thanks.

Implement persistent storage of TA time offset in OP-TEE private secure storage. TA time offset value is stored in TA storage space under file name "ta_time_offs". Fixes: OP-TEE#4577 Signed-off-by: Jarvis Dang <[email protected]>

jenswi-linaro · 2025-08-25T09:15:03Z

core/tee/tee_time_generic.c

+	if (!fops)
+		return TEE_ERROR_NOT_SUPPORTED;
+
+	res = tee_pobj_get((TEE_UUID *)uuid, (void *)TA_TIME_OFFS_ID,


The object will be created in the object namespace for the TA/UUID in question. What if a TA has an object with this name?

I think we can manage with a dummy pobj like in check_update_version() instead. This will allow us to create an object outside the normal namespace. However, we must still be careful to avoid conflicts.

jenswi-linaro · 2025-08-25T09:15:22Z

core/tee/tee_time_generic.c

+	o.positive = positive;
+
+	res = fops->create(po, true, NULL, 0, NULL, 0, &o, NULL, sizeof(o),
+		&fh);


Please align with the opening (.

jenswi-linaro · 2025-08-25T09:16:32Z

core/tee/tee_time_generic.c

+	res = tee_time_ta_set_offs_mem(uuid, offs, positive, &idx);
+	if (res)
+		return res;
+	return tee_time_ta_storage_write(uuid, offs, positive);


What if tee_time_ta_storage_write() fails? Then we have an inconsistent state.

jenswi-linaro · 2025-08-25T09:17:31Z

core/tee/tee_time_generic.c

+
+	mutex_lock(&tee_time_mtx);
+	for (n = 0; n < tee_time_num_offs; n++) {
+		if (memcmp(uuid, &tee_time_offs[n].uuid, sizeof(TEE_UUID))


Prefer if (!memcmp...).

jenswi-linaro · 2025-08-25T09:19:12Z

core/tee/tee_time_generic.c

+static TEE_Result tee_time_ta_storage_read(const TEE_UUID *uuid,
+					   TEE_Time *offs, bool *positive)
+{
+	const struct tee_file_operations *fops =


Please initialize with NULL here and assign it below for cleaner code flow.

jarvisDang · 2025-08-25T09:32:41Z

@jenswi-linaro sounds good suggestion, I will push more fixup commits soon.

github-actions · 2025-09-25T02:00:16Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

This was referenced Aug 18, 2025

TEE_SetTAPersistentTime() is not persistent accross reboots #4577

Open

TA Persistent Time is not persistent #7505

Closed

etienne-lms reviewed Aug 18, 2025

View reviewed changes

jarvisDang force-pushed the master branch from 1479e77 to 95a9133 Compare August 25, 2025 02:38

core: tee: persistent storage of TA time offset

7a0820e

Implement persistent storage of TA time offset in OP-TEE private secure storage. TA time offset value is stored in TA storage space under file name "ta_time_offs". Fixes: OP-TEE#4577 Signed-off-by: Jarvis Dang <[email protected]>

jarvisDang force-pushed the master branch from 95a9133 to 7a0820e Compare August 25, 2025 02:41

jenswi-linaro reviewed Aug 25, 2025

View reviewed changes

github-actions bot added the Stale label Sep 25, 2025

jenswi-linaro removed the Stale label Sep 25, 2025

fix open issue 4577 TEE_SetTAPersistentTime() is not persistent accro… #7506

Are you sure you want to change the base?

fix open issue 4577 TEE_SetTAPersistentTime() is not persistent accro… #7506

Uh oh!

Conversation

jarvisDang commented Aug 18, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarvisDang commented Aug 18, 2025

Uh oh!

jenswi-linaro commented Aug 20, 2025

Uh oh!

jarvisDang commented Aug 20, 2025

Uh oh!

jarvisDang commented Aug 21, 2025

Uh oh!

jarvisDang commented Aug 21, 2025

Uh oh!

jforissier commented Aug 21, 2025

Uh oh!

jarvisDang commented Aug 21, 2025

Uh oh!

jenswi-linaro commented Aug 22, 2025

Uh oh!

jarvisDang commented Aug 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarvisDang commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 25, 2025

Uh oh!

Uh oh!

jarvisDang commented Aug 25, 2025 •

edited

Loading