REE FS operation causes prefetch-abort

[ZheTing815]

Hi,

We found that a specific sequence of ree fs operations can cause issues with the use of ree_fs_dirh, which can further lead to an OS crash. For example, if there are two TAs (Trusted Applications), the TA1 calls TEE_AllocatePersistentObjectEnumerator and TEE_StartPersistentObjectEnumerator, when TEE_StartPersistentObjectEnumerator is executed, ree_fs_dirh will be created with refcount=1. Then the TA2 calls TEE_OpenPersistentObject to open a non-existent file, after os executes get_dirh, because the ree_fs_dirh has been created, the refcount will be 2. However, because it attempts to open a non-existent file, it will receive TEE_ERROR_ITEM_NOT_FOUND, and then dirh will be directly closed and released because the close flag is true. https://github.com/OP-TEE/optee_os/blob/956c2d50d60109a6053e14da6ff97d6243ea5d65/core/tee/tee_ree_fs.c#L731-L734 https://github.com/OP-TEE/optee_os/blob/956c2d50d60109a6053e14da6ff97d6243ea5d65/core/tee/tee_ree_fs.c#L670-L689 Next, when TA1 calls TEE_GetNextPersistentObject, upon reaching kernel space, it directly uses dirh without any checks at line 103, but this has already been released in the previous ree fs operation. This leads to a prefetch-abort.

E/TC:0 0 Core prefetch-abort at address 0x22e02210 (read permission fault)

https://github.com/OP-TEE/optee_os/blob/956c2d50d60109a6053e14da6ff97d6243ea5d65/core/tee/fs_dirfile.c#L96-L109

Do you have any suggestion for this problem? Perhaps we can remove the close and only use the refcount to determine whether to close dirh?

[jenswi-linaro]

Thanks for the detailed analysis, very helpful when trying to diagnose the problem.

Do you have any suggestion for this problem? Perhaps we can remove the close and only use the refcount to determine whether to close dirh?

The close parameter was added in commit:

    core: REE FS: close dirfile on error

    REE FS closes the dirfile if returning error from a function that may
    have changed the content of a secure storage object. This effectively
    undoes previous operation.

    Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
    Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
    Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

I don't think we can skip the close, but get_dirh() should check if the dirh needs to be opened again regardless of the refcounter. Does this make sense to you?

[ZheTing815]

I think that there is check in get_dirh at line 655. https://github.com/OP-TEE/optee_os/blob/956c2d50d60109a6053e14da6ff97d6243ea5d65/core/tee/tee_ree_fs.c#L653-L668 The problem is that TA1 call get_dirh in TEE_StartPersistentObjectEnumerator but uses the dirh when calling TEE_GetNextPersistentObject. Therefore, if another TA obtains the ree_fs_mutex between these two system calls of TA1, and encounters an error like TA2 which leads to the release of dirh, then TA1 will encounter an error when using dirh.

[jenswi-linaro]

Thanks, now I get it.

[ZheTing815]

The problem can be reproduced on single TA by using xtest.

diff --git a/host/xtest/regression_6000.c b/host/xtest/regression_6000.c
index 94b7498..1d98f67 100644
--- a/host/xtest/regression_6000.c
+++ b/host/xtest/regression_6000.c
@@ -1220,6 +1220,7 @@ static void xtest_tee_test_6009_single(ADBG_Case_t *c,
uint32_t storage_id)
        uint32_t obj0 = 0;
        uint32_t obj1 = 0;
        uint32_t obj2 = 0;
+       uint32_t obj3 = 0;
        uint32_t e = 0;
        uint8_t info[200] = { };
        uint8_t id[200] = { };
@@ -1266,6 +1267,12 @@ static void xtest_tee_test_6009_single(ADBG_Case_t *c,
uint32_t storage_id)
        if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_start_enum(&sess, e, storage_id)))
                goto exit;

+       /* Try to get TEE_ERROR_NOT_FOUND from open non-exist file_03 */
+       if (!ADBG_EXPECT_TEEC_RESULT(c, TEEC_ERROR_ITEM_NOT_FOUND,
+               fs_open(&sess, file_03, sizeof(file_03),
+                       TEE_DATA_FLAG_ACCESS_READ, &obj3, storage_id)))
+               goto exit;
+
        /* get 00 */
        if (!ADBG_EXPECT_TEEC_SUCCESS(c,
                fs_next_enum(&sess, e, info, sizeof(info), id, sizeof(id))))

However, I currently do not have a good solution to this problem.

[jenswi-linaro]

This should be fixed by https://github.com/OP-TEE/optee_os/pull/6898 Please let me know if you want to add a Reported-by: tag with your name and email address to the fix.

[ZheTing815]

Thank you for the quick reply. I think that the patch works.

Please let me know if you want to add a Reported-by: tag with your name and email address to the fix.

Ok, thanks! I would like to add a Reported-by: tag with my name and email address to the fix.

[ZheTing815]

Sorry for missing this. Reported-by: Gavin Liu <gavin.liu@mediatek.com>