Storage cleanup #670

jforissier · 2023-05-29T15:52:33Z

One small fix plus a new option: xtest --clear-storage.

jforissier · 2023-05-29T15:55:39Z

Here is an example on QEMU. xtest is started and interrupted, which leaves unwanted objects behind (the Ctrl-C has to happen at the right time but it's not too hard to reproduce). Therefore the next xtest run reports errors. Then xtest --clear-storage is run, which puts back the storage in a clean state as shown by the last xtest command which passes.

$ xtest 6001
Test ID: 6001
Run test suite with level=0

TEE test application started over default TEE instance
######################################################
#
# regression+pkcs11
#
######################################################

* regression_6001 Test TEE_CreatePersistentObject
o regression_6001.1 Storage id: 00000001
^C
$ xtest 6001
Test ID: 6001
Run test suite with level=0

TEE test application started over default TEE instance
######################################################
#
# regression+pkcs11
#
######################################################

* regression_6001 Test TEE_CreatePersistentObject
o regression_6001.1 Storage id: 00000001
/home/jerome/work/optee_repo_qemu_v8/out-br/build/optee_test_ext-1.0/host/xtest/regression_6000.c:801: fs_create(&sess, file_00, 0, 0x00000002 | 0x00000004, 0, data_00, sizeof(data_00), &obj, storage_id) has an unexpected value: 0xffff0003 = TEEC_ERROR_ACCESS_CONFLICT, expected 0x0 = TEEC_SUCCESS
  regression_6001.1 FAILED
o regression_6001.2 Storage id: 80000000
/home/jerome/work/optee_repo_qemu_v8/out-br/build/optee_test_ext-1.0/host/xtest/regression_6000.c:801: fs_create(&sess, file_00, 0, 0x00000002 | 0x00000004, 0, data_00, sizeof(data_00), &obj, storage_id) has an unexpected value: 0xffff0003 = TEEC_ERROR_ACCESS_CONFLICT, expected 0x0 = TEEC_SUCCESS
  regression_6001.2 FAILED
  regression_6001 FAILED
+-----------------------------------------------------
Result of testsuite regression+pkcs11 filtered by "6001":
regression_6001.1 FAILED first error at /home/jerome/work/optee_repo_qemu_v8/out-br/build/optee_test_ext-1.0/host/xtest/regression_6000.c:801
regression_6001.2 FAILED first error at /home/jerome/work/optee_repo_qemu_v8/out-br/build/optee_test_ext-1.0/host/xtest/regression_6000.c:801
regression_6001 FAILED
+-----------------------------------------------------
8 subtests of which 2 failed
1 test case of which 1 failed
131 test cases were skipped
TEE test application done!
$ xtest --clear-storage
$ xtest 6001
Test ID: 6001
Run test suite with level=0

TEE test application started over default TEE instance
######################################################
#
# regression+pkcs11
#
######################################################

* regression_6001 Test TEE_CreatePersistentObject
o regression_6001.1 Storage id: 00000001
  regression_6001.1 OK
o regression_6001.2 Storage id: 80000000
  regression_6001.2 OK
  regression_6001 OK
+-----------------------------------------------------
Result of testsuite regression+pkcs11 filtered by "6001":
regression_6001 OK
+-----------------------------------------------------
14 subtests of which 0 failed
1 test case of which 0 failed
131 test cases were skipped
TEE test application done!
$

jbech-linaro

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

jenswi-linaro · 2023-05-30T06:08:12Z

host/xtest/xtest_main.c

@@ -169,6 +171,8 @@ int main(int argc, char *argv[])
 #endif
 	else if (argc > 1 && !strcmp(argv[1], "--stats"))
 		return stats_runner_cmd_parser(argc - 1, &argv[1]);
+	else if (argc > 1 && !strcmp(argv[1], "--clear-storage"))


argc == 2 ?

jenswi-linaro · 2023-05-30T07:13:06Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

etienne-lms · 2023-05-30T10:34:42Z

ta/storage/storage.c

+		if (enum_res == TEE_ERROR_ITEM_NOT_FOUND)
+			break;
+		if (enum_res)
+			break;


This case relates to an unexpected error. I think it should be reported by the TA as its storage may not have been wiped.

Code updated to set res = enum_res;.

etienne-lms · 2023-05-30T10:35:42Z

ta/storage/storage.c

+		goto out;
+	obj_id = TEE_Malloc(TEE_OBJECT_ID_MAX_LEN, 0);
+	if (!obj_id)
+		goto out;


should set res = TEE_ERROR_OUT_OF_MEMORY

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>

ta/include/ta_storage.h uses uint32_t so it should include <stdint.h>. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Joakim Bech <joakim.bech@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>

When unexpected errors occur in the secure storage tests (regression_6xxx) some persistent objects might be left over, causing errors in further tests which expect to start from a clean state. This situation cannot be addressed fully by error handling in xtest or in the storage TA, because there are unrecoverable conditions (data abort, kill -9...). Instead, implement a new --clear-storage option which invokes the storage TA to enumerate and delete any objects it may own. The TA is invoked twice (because the same code is exposed via two UUIDS), and each invocation iterates on the two possible filesystems (TEE_STORAGE_PRIVATE_REE, TEE_STORAGE_PRIVATE_RPMB). Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Joakim Bech <joakim.bech@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>

Previously failed tests can leave some data in storage which fails the tests later on unless data is cleared. --clear-storage option is new to xtest: OP-TEE/optee_test#670 See also: https://gitlab.com/Linaro/trustedsubstrate/meta-ledge-secure/-/merge_requests/60 Ignore return values since the option may not exist in all xtest versions and it can also report failures. Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>

mwasilew · 2023-06-12T11:22:21Z

Does this also fix the problem with regression 4007_ecc and 4011 leaving the generated keys behind? I'm asking as I didn't yet have chance to test.

etienne-lms · 2023-06-12T12:11:56Z

I don't think so. This change targets ta/storageX/ TAs, not ta/crypt/ (see crypt_user_ta_uuid) regression tests 4007 and 4011 are using.
I don't think 4007/4011 use persistent objects so no cleanup required. Upon session closure, resources are released. Unless I missed somthing...

jforissier · 2023-06-12T12:21:09Z

@etienne-lms I believe @mwasilew is referring to #652 which is related to SE050. I agree that in theory those tests should not create anything persistent, but it seems with SE050 it is not what is happening. CC @ldts.

ldts · 2023-06-12T14:15:42Z

@etienne-lms I believe @mwasilew is referring to #652 which is related to SE050. I agree that in theory those tests should not create anything persistent, but it seems with SE050 it is not what is happening. CC @ldts.

yes it is a bit different with the secure element and I didnt have time to propose a fix for that yet. Controlling the persistent storage in NVM is done from its persistent storage mirror (ie, by importing the SE keys into the PKCS#11 TA database) or by using a tool like https://github.com/foundriesio/fio-se05x-cli.git.

Adressing the issue reported by those xtests is just not too high on my priority list since real SE05x users always interface to the element using PKCS#11.

jbech-linaro reviewed May 29, 2023

View reviewed changes

jforissier force-pushed the storage-cleanup branch from cea8329 to 62d44f6 Compare May 29, 2023 21:40

jenswi-linaro reviewed May 30, 2023

View reviewed changes

jforissier force-pushed the storage-cleanup branch from 3b68334 to c21e578 Compare May 30, 2023 07:17

etienne-lms reviewed May 30, 2023

View reviewed changes

jforissier force-pushed the storage-cleanup branch from 488e095 to 033e5a4 Compare May 30, 2023 11:14

etienne-lms approved these changes May 30, 2023

View reviewed changes

jforissier added 2 commits May 30, 2023 14:39

jforissier force-pushed the storage-cleanup branch from 033e5a4 to 330855f Compare May 30, 2023 12:39

mikkorapeli-linaro mentioned this pull request May 31, 2023

optee-xtest.yml: clear storage before executing tests Linaro/test-definitions#444

Merged

jforissier merged commit aa43c90 into OP-TEE:master May 31, 2023
1 check passed

jforissier deleted the storage-cleanup branch May 31, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage cleanup #670

Storage cleanup #670

jforissier commented May 29, 2023

jforissier commented May 29, 2023

jbech-linaro left a comment

jenswi-linaro May 30, 2023

jforissier May 30, 2023

jenswi-linaro commented May 30, 2023

etienne-lms May 30, 2023

jforissier May 30, 2023

etienne-lms May 30, 2023

jforissier May 30, 2023

etienne-lms left a comment

mwasilew commented Jun 12, 2023

etienne-lms commented Jun 12, 2023 •

edited

Loading

jforissier commented Jun 12, 2023

ldts commented Jun 12, 2023

Storage cleanup #670

Storage cleanup #670

Conversation

jforissier commented May 29, 2023

jforissier commented May 29, 2023

jbech-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro May 30, 2023

Choose a reason for hiding this comment

jforissier May 30, 2023

Choose a reason for hiding this comment

jenswi-linaro commented May 30, 2023

etienne-lms May 30, 2023

Choose a reason for hiding this comment

jforissier May 30, 2023

Choose a reason for hiding this comment

etienne-lms May 30, 2023

Choose a reason for hiding this comment

jforissier May 30, 2023

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

mwasilew commented Jun 12, 2023

etienne-lms commented Jun 12, 2023 • edited Loading

jforissier commented Jun 12, 2023

ldts commented Jun 12, 2023

etienne-lms commented Jun 12, 2023 •

edited

Loading