Secure Storage: storage size is not updated after TEE_TruncateObjectData

[kevin-peng-hao]

I’m working with the OP-TEE release 2.6.0.
But I found that the TEE_TruncateObjectData does not behave as expected:
The truncated size is not write to storage meta.

To be more detailed:

1. Open or create a persistent object
2. Write some data into the object
3. Truncate the data
4. Without close the object, read out content, it's correctly the truncated data whether the new size is larger or less then the original one.
5. But once you close and re-open the object, the read out data size is not what you have truncated

So probably the truncated size is not saved to the storage, and when the object is reopened and the data size read is the old one.

We also found some test cases about TEE_TruncateObjectData, but it doesn't close and reopen the object. If we modified the case to that it close the object and open it and then read it, we got the same result.

I traced the code to ree_fs_truncate() in tee_ree_fs.c and did find some bugs:

	res = ree_fs_ftruncate_internal(fdp, len);

	if (!res)
		goto out;

	res = tee_fs_htree_sync_to_storage(&fdp->ht, fdp->dfh.hash);
	if (!res)
		goto out;

	res = tee_fs_dirfile_update_hash(dirh, &fdp->dfh);

The if (!res) should be if (res), right?
But this is only the part of the issue. The tee_fs_htree_sync_to_storage still didn't sync the data size to storage, because !ht->dirty:

TEE_Result tee_fs_htree_sync_to_storage(struct tee_fs_htree **ht_arg,
					uint8_t *hash)
{
	TEE_Result res;
	struct tee_fs_htree *ht = *ht_arg;
	size_t size;
	void *ctx;

	if (!ht)
		return TEE_ERROR_CORRUPT_OBJECT;

	if (!ht->dirty)
		return TEE_SUCCESS;

I'm somehow not allowed to go deep further.
So I raised the issue here.

Thanks,
Kevin

[jforissier]

The if (!res) should be if (res), right?

Yes.

The tee_fs_htree_sync_to_storage still didn't sync the data size to storage, because !ht->dirty:

My understanding is that if the truncate operation changes only meta->length but does not change the number of underlying blocks, dirty is not set. Maybe the htree layer should provide a function to be called when meta are modified by the FS layer? @jenswi-linaro what do you think?

If we modified the case to that it close the object and open it and then read it, we got the same result.

Such a modification should definitely be added to xtest IMO.

[jforissier]

@kevin-peng-hao does #2099 fix the problem?

[kevin-peng-hao]

@jforissier No, it doesn't. Actual if the new truncate size is larger than the old size, it works fine.
But it doesn't works the reverse.

[jforissier]

@kevin-peng-hao can you share a patch for xtest to reproduce the issue?

[kevin-peng-hao]

Sure. BTW, actually neither truncate or extend works if close & reopen. Sorry for the wrong conclusion earlier.

The patch is based on optee_test release 2.6.0

---
 host/xtest/regression_6000.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/host/xtest/regression_6000.c b/host/xtest/regression_6000.c
index b987bf6..db99697 100644
--- a/host/xtest/regression_6000.c
+++ b/host/xtest/regression_6000.c
@@ -482,6 +482,29 @@ static void test_truncate_file_length(ADBG_Case_t *c, uint32_t storage_id)
 	/* check buffer */
 	(void)ADBG_EXPECT_BUFFER(c, &data_00[5], 5, out, count);
 
+	/* close */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_close(&sess, obj)))
+		goto exit;
+
+	/* open */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_open(&sess,  file_01, sizeof(file_01),
+			TEE_DATA_FLAG_ACCESS_WRITE |
+			TEE_DATA_FLAG_ACCESS_READ |
+			TEE_DATA_FLAG_ACCESS_WRITE_META, &obj, storage_id)))
+		goto exit;
+
+	/* seek */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(
+		    c, fs_seek(&sess, obj, 5, TEE_DATA_SEEK_SET)))
+		goto exit;
+
+	/* verify */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_read(&sess, obj, out, 10, &count)))
+		goto exit;
+
+	/* check buffer */
+	(void)ADBG_EXPECT_BUFFER(c, &data_00[5], 5, out, count);
+
 	/* clean */
 	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_unlink(&sess, obj)))
 		goto exit;
@@ -531,6 +554,31 @@ static void test_extend_file_length(ADBG_Case_t *c, uint32_t storage_id)
 	expect[1] = data_00[31];
 	(void)ADBG_EXPECT_BUFFER(c, &expect[0], 10, out, count);
 
+	/* close */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_close(&sess, obj)))
+		goto exit;
+
+	/* open */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_open(&sess,  file_01, sizeof(file_01),
+			TEE_DATA_FLAG_ACCESS_WRITE |
+			TEE_DATA_FLAG_ACCESS_READ |
+			TEE_DATA_FLAG_ACCESS_WRITE_META, &obj, storage_id)))
+		goto exit;
+
+	/* seek */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(
+		    c, fs_seek(&sess, obj, 30, TEE_DATA_SEEK_SET)))
+		goto exit;
+
+	/* verify */
+	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_read(&sess, obj, out, 10, &count)))
+		goto exit;
+
+	/* check buffer */
+	expect[0] = data_00[30];
+	expect[1] = data_00[31];
+	(void)ADBG_EXPECT_BUFFER(c, &expect[0], 10, out, count);
+
 	/* clean */
 	if (!ADBG_EXPECT_TEEC_SUCCESS(c, fs_unlink(&sess, obj)))
 		goto exit;
-- 

[kevin-peng-hao]

Hi @jforissier Any updates on this issue?

[jenswi-linaro]

Hi @kevin-peng-hao, I'm looking at this too. I have one additional fix. I have a pretty good idea of what more needs to be done, but since it's not complete yet I don't know if it's enough.

[kevin-peng-hao]

Thanks, Jens @jenswi-linaro . Glad to know you guys are working on it. Thanks.

[jforissier]

@kevin-peng-hao can you test again with #2099? Should be OK now.

[kevin-peng-hao]

Thanks @jforissier It's been fixed.
BTW, are you going to update the xtest of testing this issue.

[jforissier]

@kevin-peng-hao yes, I will merge the fix (2099) and the xtest update (249).

[kevin-peng-hao]

Great, thanks. @jforissier