core: rpmb: __weak access implementation selectable at build time #5323
Conversation
core/tee/tee_rpmb_fs.c
Outdated
| @@ -3081,5 +3081,8 @@ TEE_Result tee_rpmb_fs_raw_open(const char *fname, bool create, | |||
|
|
|||
| bool __weak plat_rpmb_key_is_ready(void) | |||
| { | |||
| return true; | |||
| if (IS_ENABLED(CFG_RPMP_IS_READY_WEAK)) | |||
There was a problem hiding this comment.
Wasn't the point with the __weak implementation that it should be easily overridden?
There was a problem hiding this comment.
yes, well kind of: we should not have to modify the source code to override the platform's weak behavior: some platforms might want to return false to prevent the RPMB persistent keys to be written (or even just return false for some builds for the same platform).
There was a problem hiding this comment.
it is often the case that the RPMB keys depend on the SoC security state so having a more restricted behavior for the weak implementation seems sensible to avoid being locked out.
There was a problem hiding this comment.
It seems that the behaviour you're looking for is actually platform specific. The point with this __weak function is that the platform should override it if it needs anything but the default behaviour.
I suggest that you provide this function in the platform you're referring to.
Another option to achieve the result you're looking for is to compile with CFG_RPMB_FS=n
There was a problem hiding this comment.
ok, why is the default being set to true? I am arguing it should be set to false by default but since I do not want to break backwards compatibility I am proposing this alternative.
If you can provide a solid argument for the current default that is all I need. I do not think you can.
There was a problem hiding this comment.
It looks like the documentation should be updated. https://optee.readthedocs.io/en/latest/architecture/secure_storage.html#important-caveats and https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html?highlight=CFG_RPMB_WRITE_KEY#op-tee-build-instructions could be more clear.
yes I can do that while is still fresh in my mind. will post and update today (unless @Emantor wants to do it)
There was a problem hiding this comment.
@ldts please do, I am at a conference ATM.
There was a problem hiding this comment.
@jenswi-linaro OP-TEE/optee_docs#157 not sure if this is enough or if you wanted a different angle to the issue we have been discussing
There was a problem hiding this comment.
@ldts please do, I am at a conference ATM.
ah nice - we, Foundries, will be at ELC Austin next month in case anyone wants to meet
Writing the RPMB key is now disabled when using the default platform behaviour. Please refer to the Secure Storage section of the online OP-TEE documents for additional information. Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
| @@ -3081,5 +3081,5 @@ TEE_Result tee_rpmb_fs_raw_open(const char *fname, bool create, | |||
|
|
|||
| bool __weak plat_rpmb_key_is_ready(void) | |||
| { | |||
| return true; | |||
| return false; | |||
There was a problem hiding this comment.
If plat_rpmb_key_is_ready() is changed to always return false, then must all platforms that wishes to use RPMB override that function. If so then we could just as well remove it and require the platform to provide it if CFG_RPMB_WRITE_KEY=y. That will for sure trigger a lot of questions.
Overriding plat_rpmb_key_is_ready() is in my opinion only useful if there's some hardware state that can be checked to tell if it seems that the generated key will be the final key.
There was a problem hiding this comment.
Yes if the default is set to false, it will impact current users. But for some it might be for the best, protecting their RPMB hardware. For others is an inconvenience with no monetary cost...which could be worked around with an additional configuration.
When you work enabling hardware on different platforms, the current situation is nasty: securing a board is not a trivial activity and the current default (true) just adds pain to injury.
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
I am closing this PR. |
Avoid having to patch the source code to prevent accessing RPMB by
mistake.
Signed-off-by: Jorge Ramirez-Ortiz [email protected]