secure storage: update head message when info->dataSize update #1217
Conversation
| res = fops->read(o->fh, &head, &bytes); | ||
| if (res != TEE_SUCCESS) { | ||
| if (res == TEE_ERROR_CORRUPT_OBJECT) | ||
| EMSG("Head corrupt.\n"); |
|
With or without my comment fixed. |
|
|
||
| /* read head */ | ||
| bytes = sizeof(struct tee_svc_storage_head); | ||
| res = fops->read(o->fh, &head, &bytes); |
There was a problem hiding this comment.
You could skip the read step if you'd only update the position field in the file.
There was a problem hiding this comment.
But should we assert that head.magic == TEE_SVC_STORAGE_MAGIC? If not need, I think we can skip the read step.
There was a problem hiding this comment.
I think we're good without that assert.
lackan
force-pushed
the
fix-persistent-objects
branch
from
106c48e to
bf93c00
Compare
| @@ -1069,8 +1098,10 @@ TEE_Result syscall_storage_obj_write(unsigned long obj, void *data, size_t len) | |||
| goto exit; | |||
|
|
|||
| o->info.dataPosition += len; | |||
| if (o->info.dataPosition > o->info.dataSize) | |||
| if (o->info.dataPosition > o->info.dataSize) { | |||
| o->info.dataSize = o->info.dataPosition; | |||
There was a problem hiding this comment.
update dataPosition and dataSize only if successfully updated the head ?
| @@ -1069,8 +1097,12 @@ TEE_Result syscall_storage_obj_write(unsigned long obj, void *data, size_t len) | |||
| goto exit; | |||
|
|
|||
| o->info.dataPosition += len; | |||
There was a problem hiding this comment.
I don't think this should be updated if we can't update head below.
There was a problem hiding this comment.
But the write operation have overwrote the data, the data position actually have changed? So we ignore this overwrite?
There was a problem hiding this comment.
Hmm, this is actually a problem with the way we're layering this. The update in the lower layer is already committed and what we would need now is unrolling it. This problem is out of scope of this P-R so please ignore my previous comment for now.
|
@lackan did @jenswi-linaro give is R-b? I can't see it in the discussion, yet you added it to your first commit. @jenswi-linaro please let us know if you're OK with the current state. Also, @lackan could you please add a test to xtest for this case? |
|
Fine with me: |
|
@jforissier Yes, I will add a test to xtest soon. |
When one invokes TEE_WriteObjectData and write some data into the secure storage file, the data size of the file may change, but currently it will not update the head message in persistent objects. This commit will fix this problem by updating head message when info->dataSize update. Signed-off-by: Guanchao Liang <[email protected]> Reviewed-by: Joakim Bech <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>
lackan
force-pushed
the
fix-persistent-objects
branch
from
e47ccc0 to
fbea8bc
Compare
|
Rebased on top of master and merged. Thanks @lackan. |
ghost
mentioned this pull request
When one invokes TEE_WriteObjectData and write some data into the secure storage file, the data size of the file may change, but currently it will not update the head message in persistent objects. This commit will fix this problem by updating head message when info->dataSize update.
Relates to:
#814
Output:(test on FVP)
Test for xtest have add to OP-TEE/optee_test#151
Signed-off-by: Guanchao Liang [email protected]
Reviewed-by: Joakim Bech [email protected]
Reviewed-by: Jens Wiklander [email protected]