OP-TEE / optee_os

Trusted side of the TEE
Other
1.59k stars 1.07k forks source link

Potenssial issue in persistent_token.c::init_persistent_db() #6977

Closed weizhaojiang closed 2 months ago

weizhaojiang commented 3 months ago

We are investigating an issue with the init_persistent_db() function from the OP-TEE repository, which leads to a panic at the following code segment:

        size = sizeof(*db_objs);
        res = TEE_ReadObjectData(db_hdl, db_objs, size, &size);
        if (res || size != sizeof(*db_objs))
            TEE_Panic(0);

While the database file can be successfully opened and db_main can be read with the expected size, the call to TEE_ReadObjectData(db_hdl, db_objs, size, &size) returns a size of 0. Our analysis suggests that this issue could arise if there was a power loss between writing db_main and db_objs.

Since db_objs should never be empty, we propose writing both db_main and db_objs in a single operation during TEE_CreatePersistentObject(), rather than writing them separately. What are your thoughts on this approach?

weizhaojiang commented 3 months ago

To initialize a token database when it doesn’t exist, we follow these two steps:

  1. Call TEE_CreatePersistentObject() with db_main as the initial data.
  2. Truncate the object data and then insert db_obj data.

We've conducted an experiment to confirm our earlier suspicions. We introduced a 5-second delay between step 1 and step 2 and intentionally powered off the device during this period. Our findings show that if a power loss occurs between these steps, only the db_main data is present in the database. When the device restarts, it detects the existing database and successfully reads db_main, but fails to read db_obj, leading to a TA panic each time.

One possible solution is to write both db_main and the initial 4 bytes of db_objs (with a count of 0) in a single operation during TEE_CreatePersistentObject(). This minor adjustment could resolve the issue.

Please review this suggestion and share any thoughts you may have. Thanks.

weizhaojiang commented 3 months ago

Although this is a very rare situation, this issue could become more prevalent and become a significant concern if there is a large number of devices experiencing random power losses.

jenswi-linaro commented 3 months ago

It sounds like you have a fix for the problem. Would you mind creating a PR with the fix so we can review it?

weizhaojiang commented 3 months ago

Yes, I’ve implemented a fix and verified it with local tests. I’ll go ahead and submit it for review.

weizhaojiang commented 2 months ago

the patch has been merged, closing the ticket.