core: do not access RPMB storage if accessing from REE fops.

[wangjudyw]

If we enable CFG_RPMB_FS and CFG_REE_FS at the same time. A call from
ree_fs_open() will falsely accessing RPMB to get directory handle by
calling get_dirh() to open_dirh().

How-tested: execute xtest -t regression with optee-os CFG_REE_FS=y
and CFG_RPMB_FS=y. Testcase 1004 will fail.

Actually I don't understand why keeping a path CFG_RPMB_FS inside
tee_ree_fs.c. In our case we need both RPMB and REE fs enabled
because we want to test if the platform has been provisioned
with RPMB key. But we don't want to use RPMB for secure storage
just yet. So I need to keep both CFG_RPMB_FS and CFG_REE_FS
enabled and let storage id choose which applies.

Signed-off-by: Judy Wang wangjudy@microsoft.com

[jforissier]

Hello @wangjudyw,

If we enable CFG_RPMB_FS and CFG_REE_FS at the same time. A call from ree_fs_open() will falsely accessing RPMB to get directory handle by calling get_dirh() to open_dirh().

That's expected.

How-tested: execute xtest -t regression with optee-os CFG_REE_FS=y and CFG_RPMB_FS=y. Testcase 1004 will fail.

Works fine here on QEMU with make -j10 CFG_RPMB_FS=y CFG_REE_FS=y CFG_RPMB_WRITE_KEY=y CFG_RPMB_TESTKEY=y run.

Actually I don't understand why keeping a path CFG_RPMB_FS inside tee_ree_fs.c.

Because RPMB is used for integrity and anti-rollback of the REE FS storage, see commit b4b1a20.

In our case we need both RPMB and REE fs enabled because we want to test if the platform has been provisioned with RPMB key. But we don't want to use RPMB for secure storage just yet. So I need to keep both CFG_RPMB_FS and CFG_REE_FS enabled and let storage id choose which applies.

It looks like what you want is to make the RPMB access configurable via some new config symbol, for example CFG_REE_FS_INTEGRITY_RPMB?

[wangjudyw]

we are relying on AVB pta command TA_AVB_CMD_READ_PERSIST_VALUE to attempt a secure storage read. In AVB TA, storage id is fixed to TEE_STORAGE_PRIVATE_RPMB. If optee-os supports RPMB fs and is provisioned, it'll return TEE_SUCCESS. If not, it'll return TEE_ERROR_STORAGE_NOT_AVAILABLE from tee_rpmb_write_and_verify_key().

For now we just want to check whether platforms are provisioned with RPMB key, but still use REE as secure storage fs. With that assumption, if we keep CFG_RPMB_FS=n while CFG_REE_FS=y. tee_svc_storage_file_ops() will return NULL for AVB TA read, and being translated to TEE_ERROR_ITEM_NOT_FOUND in syscall_storage_obj_open()

We don't need RPMB fs to work, but just want to verify key provisioning. Any other suggestions are very welcome.
Thank you.

[jforissier]

For now we just want to check whether platforms are provisioned with RPMB key, but still use REE as secure storage fs.

This is what will happen with CFG_REE_FS=y CFG_RPMB_FS=y.

• As you said, the AVB TA uses TEE_STORAGE_PRIVATE_RPMB so it will use RPMB
• The default storage (TEE_STORAGE_PRIVATE) will be REE FS because it has a higher priority than RPMB (see here)
• If it bothers you that the REE FS stores some integrity meta-data in RPMB, then your patch is needed but please add a separate config symbol, because we don't want to lose the abillity to do rollback detection for the REE FS storage. See below.

[jforissier]

#if defined(CFG_RPMB_FS) && defined(CFG_REE_FS_INTEGRITY_RPMB)

[wangjudyw]

I forgot to mention our tee-supplicant only supports REE, that's why xtest fail in this case. If someone like us using optee-os w/ CFG_RPMB_FS=y, CFG_REE_FS=y, but optee-client uses CFG_REE_FS=y. This is what will happen.
Because optee-os in our case is in firmware, but optee-client runs in Linux kernel user space. It's hard to always guarantee both sides match. So with your suggestion I added option CFG_REE_FS_INTEGRITY_RPMB and default enabled it. Platforms like ours can set it to false if we don't need REE fs roll-back protection.

The original patch is buggy since it will make calls to ree_fs_open() go without roll-back protection. So it's been revised.
Thank you.

[jforissier]

Hi @wangjudyw, thanks for the update. I think I understand your use case now: your tee-supplicant cannot access RPMB, but you still enable RPMB in optee_os because at some point during boot some other firmware component is providing RPMB access and plays the role of tee-supplicant, is that correct? If so, it would be nice to explain because from your commit description it is hard to understand why you build optee_client and optee_os with different settings.

One more comment below.

[wangjudyw]

just updated commit comments. hope that makes sense. thx!

[wangjudyw]

After reviewing optee-client on our platforms, I found a misunderstanding about tee-supplicant accessing RPMB therefore commit description is modified again.

The misunderstanding is that, there's no CFG_RPMB_FS or CFG_REE_FS at all in optee-client for tee-supplicant. Whether tee-supplicant could access RPMB or not solely depends on the Linux kernel we ran. Optee driver in Linux kernel should provide the functionality when tee-supplicant is trying to access RPMB thru ioctl(). However we have an existing problem of optee driver in Linux kernel that makes our platform failed to support RPMB. It's a known issue between Linux optee driver community and won't be solved in a short time. That's why even with xtest and optee-os setting CFG_RPMB_FS=y, tee-supplicant will return errors on our platforms.

But due to our production process, the platform needs to support an unittest to check if key for RPMB is provisioned or not. So we have to make CFG_REE_FS=y and CFG_RPMB_FS=y in optee-os, but don't care whether we could access contents in RPMB.

[jforissier]

Hi @wangjudyw,

Thanks for updating the explanation. These things are important for us reviewer/maintainers because it helps understand the reason why the code is as it is, and what could happen if we modify it.

I have one last remark below. With that fixed:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

[wangjudyw]

updated, thx!

[wangjudyw]

if no other concerns can we close and merge the patch? We have remaining works depending on this. Thank you.

[jforissier]

@wangjudyw you're expected to add the Reviewed-by/Acked-by tags as mentioned in https://optee.readthedocs.io/en/latest/general/contribute.html#finalizing-your-contribution. This is meant to reduce the amount of work for me when merging your PR. But no worries, I have added the tag and merged manually. Now you will know for your next contribution 😉