Risk of removal object unexpectedly

[zhupg]

Hello, There is risk that object is removed unexpectedly in error handling in tee_svc_storage_read_head(). https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_svc_storage.c#L138-L143

        res = fops->read(o->fh, sizeof(struct tee_svc_storage_head),
                 attr, NULL, &bytes);
        if (res == TEE_ERROR_OUT_OF_MEMORY)
            goto exit;
        if (res != TEE_SUCCESS || bytes != head.attr_size)
            res = TEE_ERROR_CORRUPT_OBJECT;
        if (res)
            goto exit;

It treats all errors as TEE_ERROR_CORRUPT_OBJECT, except TEE_ERROR_OUT_OF_MEMORY. However it should validate the data from RPMB, only if validation failed, the object is corrupt. For example, if there is error when preparing to read RPMB in tee_rpmb_req_pack(), there is no corruption actually and the error should not be translated to TEE_ERROR_CORRUPT_OBJECT.

I think we should change it like this.

        res = fops->read(o->fh, sizeof(struct tee_svc_storage_head),
                 attr, NULL, &bytes);
        if (bytes != head.attr_size)
            res = TEE_ERROR_CORRUPT_OBJECT;
        if (res) {
            if (res == TEE_ERROR_CORRUPT_OBJECT)
                EMSG("Head meta corrupt");
            goto exit;
        }

Could you please confirm this risk? thanks.

[jenswi-linaro]

In your proposed change to report TEE_ERROR_OUT_OF_MEMORY as TEE_ERROR_CORRUPT_OBJECT, that's not right.

Can tee_rpmb_req_pack() realistically return any other error than TEE_ERROR_OUT_OF_MEMORY?

However, I see your concern. The normal world might trigger OP-TEE to remove objects by altering the data returned. Perhaps we can identify a few cases where TEE_ERROR_STORAGE_NOT_AVAILABLE would be a better choice.

[zhupg]

Thanks for your reply.

In my change, it does not report TEE_ERROR_OUT_OF_MEMORY as TEE_ERROR_CORRUPT_OBJECT, it just return the original error code.

I am not sure is it possible to return any other error than TEE_ERROR_OUT_OF_MEMORY in tee_rpmb_req_pack() in some special circumstances. But I saw this error 'tee_rpmb_invoke failed, res = 0xffff000e' when tee-supplicant is shutdown or restart abnormally, and it caused object corrupt.

The RPMB reading process is complex, including crypto, RPC and eMMC driver, there are many error handling in the code, but not all of the error is caused by object corrupt. Wrong handling will trigger OP-TEE to remove objects. Similarly, as you mentioned, the normal world can trigger OP-TEE to remove objects.

I suggest making this change because I have seen other places handle it the same way. https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_svc_storage.c#L100-L105 https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_svc_storage.c#L752-L759 Only the code I pointed(L138-L143) translate all errors to TEE_ERROR_CORRUPT_OBJECT.

I am not sure whether It is easy to identify the cases where TEE_ERROR_STORAGE_NOT_AVAILABLE and whether it can cover all the case.

[jenswi-linaro]

Feel free to propose the changes you need in a PR.

[zhupg]

I have created a PR https://github.com/OP-TEE/optee_os/pull/7005. We can reproduce the object corruption issue with error code TEE_ERROR_COMMUNICATION in stress reboot test. With this patch, we can not reproduce it any more. Please help to review.