RPMB: support UFS - #7881
Conversation
f09900c to
c7d1df8
Compare
| else | ||
| rpmb_ctx->rel_wr_blkcnt = dev_info->rel_wr_sec_c; | ||
| if (!rpmb_ctx->rel_wr_blkcnt) | ||
| rpmb_ctx->rel_wr_blkcnt = 1; |
There was a problem hiding this comment.
Or should we return an error?
There was a problem hiding this comment.
sure I can do that as well (probably better than defaulting)
| break; | ||
| /* Provision the key over the subsystem interface itself. */ | ||
| if (IS_ENABLED(CFG_RPMB_WRITE_KEY) && | ||
| res == TEE_ERROR_ITEM_NOT_FOUND) { |
There was a problem hiding this comment.
So we program the key into the first RPMB partition without a programmed key, without even checking whether we might already have a key programmed for a later partition?
That might actually happen if U-Boot uses a different probe order than the kernel.
I don't like this automatic programming. It seems risky in more than one way.
There was a problem hiding this comment.
@jenswikl do you have an RPMB emulator? I already burn the key on the device I was testing with (didnt think of this possibility); just proposed an alternative to the above issue.
There was a problem hiding this comment.
I proposed a simple implementation - ie picking the first free slot if none are provisioned; we could also add a config that searches for a specific ID to provision (emmc id or the proposed blake2b crypto hash for UFS)
There was a problem hiding this comment.
@jenswikl do you have an RPMB emulator? I already burn the key on the device I was testing with (didnt think of this possibility); just proposed an alternative to the above issue.
No, I don't. I use a fresh RPMB and keep my fingers crossed whenever I need to test a new way of programming a key.
I understand that this automated programming is very convenient, but I don't think we pay enough attention to its drawbacks.
In my opinion, we should move away from automated programming in OP-TEE instead of extending it further. Selecting which RPMB device to use should be done during production and not be hardcoded into OP-TEE.
There was a problem hiding this comment.
I couldn't agree more. At least we should have a config with the device unique id to provision and use
There was a problem hiding this comment.
The issue you originally raised was device selection: with several unprovisioned RPMB devices, OP-TEE just attempts to provision the first. This PR already improves that — it skips already provisioned devices and puts selection in one function that's a natural hook for a smarter algorithm (and optionally a CFG_ device-id selector for deterministic selection).
To be fair, it introduced the problem too.
The broader point you're now raising - automated vs. an explicit provisioning PTA gated by CFG_RPMB_WRITE_KEY - I agree with, and it's what the QC teams are pursuing along the lines of #7787; Id could help align this code once it lands.
Aha, that's good news indeed.
What I don't follow is why that redesign should block this PR: it only makes the existing RPMB flow work on UFS-only platforms; so muy question is, could we treat UFS enablement and the provisioning redesign as two separate efforts?
Fair enough, let's go ahead with this PR. Is anything else missing here?
There was a problem hiding this comment.
I havent added the CFG_ for deterministic selection of the device to provision - should I do that?
Also maybe lets not merge until Linux agree on the ABI change. Just wanted to be sure we wont have to do a big rework when that happens
There was a problem hiding this comment.
I havent added the CFG_ for deterministic selection of the device to provision - should I do that?
It's a good idea and protects well against accidental programming. I welcome it if it isn't too much trouble.
Also maybe lets not merge until Linux agree on the ABI change. Just wanted to be sure we wont have to do a big rework when that happens
Makes sense.
There was a problem hiding this comment.
the ABI change was accepted. I'll add the CFG_ support as a conditional https://lkml.org/lkml/2026/8/26/1421
There was a problem hiding this comment.
I added a CFG_ commit on top - @jenswikl please let me know if you agree with this functionality (I described it above but I am not sure I might have explained it properly when you agreed).
94fbc61 to
f6cae9a
Compare
|
|
||
| /* current trivial algorithm is to pick the first candidate */ | ||
| assert(cand); | ||
| memcpy(rpmb_ctx, &cand->ctx, sizeof(*rpmb_ctx)); |
There was a problem hiding this comment.
I think this API should use rpmb_lock as it is overwriting rpmb_ctx.
There was a problem hiding this comment.
I am looking at below two threads:
Thread 1: tee_rpmb_write_blk() → holds rpmb_mutex → issues RPMB write request → suspends to normal world at tee_rpmb_invoke() .
Thread 2:
▎ PTA_CMD_GET_DEVICES_RPMB → tee_rpmb_reinit() → tee_rpmb_init() → rpmb_ctx overwritten.
Thread 1's mutex excludes nothing because Thread 2 never acquires it.
There was a problem hiding this comment.
seems to me that what you are reporting is an issue upstream that this patch is just inheriting. would you mind sending a fix?
There was a problem hiding this comment.
all right, I took care of fixing it upstream - just added to my series. In the future please feel free to fix issues you see upstream directly
| @@ -242,11 +242,16 @@ struct rpmb_raw_data { | |||
| }; | |||
|
|
|||
| #define RPMB_EMMC_CID_SIZE 16 | |||
There was a problem hiding this comment.
We can remove this macro and use RPMB_CID_SIZE, any reason why this macro is retained?
There was a problem hiding this comment.
thought it carries no functional value it serves a purpose - it is a lightweight way of documenting it in a maintainable manner as opposed to writing a comment in the code explaining why we have chosen 16.
| if (!IS_ENABLED(CFG_RPMB_WRITE_KEY)) | ||
| continue; | ||
|
|
||
| res = add_rpmb_ctx_to_list(); |
There was a problem hiding this comment.
Why do we need to add more than one, and why do we need to keep the list in a global variable?
There was a problem hiding this comment.
We do not need to add more than one if we just pick the first one available: I figured I'd provide a simple way to let users extend to other criteria - thought I didnt provide a hook for algorithms...shall I remove the list then and pick the first one?
no need for global. I was being lazy. will fix.
There was a problem hiding this comment.
We do not need to add more than one if we just pick the first one available: I figured I'd provide a simple way to let users extend to other criteria - thought I didnt provide a hook for algorithms...shall I remove the list then and pick the first one?
yes I'll get rid of the list - we'll pick one that is available.
| } | ||
| res = tee_rpmb_reinit(); | ||
| if (res) | ||
| return TEE_ERROR_STORAGE_NOT_AVAILABLE; |
There was a problem hiding this comment.
Without RPMB we're returning an error instead of an empty list and TEE_SUCCESS.
Why do we want this?
There was a problem hiding this comment.
I'll have to look into this - I didn't modify the functionality on this bit.
There was a problem hiding this comment.
um you are right - I did modify the behaviour- need to check more carefully,
There was a problem hiding this comment.
with CFG_RPMB_FS disabled, tee_rpmb_reinit always errors, so the empty-list case never survived. Gated on CFG_RPMB_FS now. I think this addresses your concern.
The RPMB device context is global state shared by every caller. The filesystem paths already coordinate access to it through a lock, and they rely on the assumption that this context is only read or modified while that lock is held. Device enumeration reaches the very same context, and triggers its initialization, on a path that does not honour that assumption. It can therefore execute at the same time as a filesystem operation running on another thread, leaving two callers reading and writing the shared context simultaneously. The resulting data race can corrupt the context and leave the RPMB subsystem in an inconsistent state. Fixes: 8dfdf39 ("core: rpmb: probe for kernel RPMB driver") Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
OP-TEE configuration switches use the CFG_ prefix; the multiple-write-fixed build guard does not follow that convention. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
The CID size is hardcoded to the eMMC value throughout the RPMB code. Supporting other RPMB devices requires dropping that eMMC-specific assumption. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
Platforms whose RPMB lives on UFS could only reach it through the legacy single-command interface, which forced CFG_RPMB_WRITE_KEY to stay enabled. The probe interface should serve UFS too so those platforms no longer depend on the legacy path. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
Forcing key provisioning through the legacy path prevents a normal world that implements only the RPMB subsystem interface from programming the key. It should be possible to provision over the subsystem interface so those systems can provision and use RPMB without a legacy fallback. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
PTA_CMD_GET_DEVICES_RPMB skipped re-probing RPMB whenever CFG_REE_FS was configured. CFG_REE_FS does not exclude CFG_RPMB_FS though, so on a build with both enabled the command failed to re-probe RPMB. Gate the re-probe on CFG_RPMB_FS instead, so RPMB is re-initialized whenever the RPMB filesystem is configured, independent of CFG_REE_FS. On a build without RPMB (CFG_REE_FS only) the command now falls through to an empty device list with TEE_SUCCESS, instead of unconditionally returning TEE_ERROR_STORAGE_NOT_AVAILABLE. Validated on SA8775P (CFG_REE_FS=y, CFG_RPMB_FS=y): U-Boot accesses RPMB during boot, then Linux accesses the same RPMB after boot -- both work. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
When several RPMB-capable devices are present, key provisioning must not land on an arbitrary one: writing the key to the wrong device is hard to undo and leaves the intended device unprovisioned. CFG_RPMB_WRITE_KEY_CID lets a controlled (factory) setup pin provisioning to a known device so the outcome is deterministic. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
| break; | ||
| case PTA_CMD_GET_DEVICES_RPMB: | ||
| if (!IS_ENABLED(CFG_REE_FS)) { | ||
| if (IS_ENABLED(CFG_RPMB_FS)) { |
There was a problem hiding this comment.
I understand that you want to call tee_rpmb_reinit() while probing the kernel driver to tell OP-TEE there's a new normal world driver and RPMB needs to be reinitialized to work.
But doing this while CFG_REE_FS=y might cause the tee-bus to probe TAs that rely on tee-supplicant to be available. PTA_CMD_GET_DEVICES_RPMB is called before tee-supplicant is guaranteed to be ready.
We need a way for the driver to tell OP-TEE to reinitialize RPMB the next time it attempts to access it. We don't need a full tee_rpmb_reinit() during probe. It's enough to set rpmb_ctx->reinit to true, but we need to find a good spot. Ideally, we should do it as a yielding call so we can take the rpmb_mutex.
A most hackish way would be something like:
if (!IS_ENABLED(CFG_RPMB_FS) || IS_ENABLED(CFG_RPMB_FS)) {
res = tee_rpmb_reinit();
if (res)
return TEE_ERROR_STORAGE_NOT_AVAILABLE;
}
if (!IS_ENABLED(CFG_REE_FS))
rflags = TA_FLAG_DEVICE_ENUM_TEE_STORAGE_PRIVATE;The advantage is that we reuse this round trip to the secure world during boot. We don't need to change the kernel driver, and minimal changes here. But it also means that for RPMB to work properly, we depend on the normal-world driver to implement a tee bus.
OP-TEE RPMB on UFS — the key-derivation ABI
This PR lets OP-TEE use RPMB secure storage on UFS-only platforms (no eMMC).
RPMB security rests on an authentication key that OP-TEE derives in the secure
world from its HUK and a device id (
dev_id) handed down by the normal world:OP-TEE's flow was written for eMMC, where
dev_idis the 16-byte eMMC CID.To reuse that flow unchanged for UFS, the normal world presents a
dev_idwiththe same fixed 16-byte layout by hashing the (variable-length) UFS identifier.
The ABI
OP-TEE consumes
dev_idverbatim (from MMC or UFS) — it does no hashing itself. Every normalworld that drives OP-TEE RPMB on UFS therefore has to derive the same
dev_id,or the derived key changes and the RPMB becomes inaccessible (or is provisioned
with a key the other world cannot reproduce):
"<device_id>-R<region>"device_idis the kernel'sufshcd_create_device_id(); onedev_idper RPMB regionWho agrees on it
flowchart TB UB["U-Boot — boot-time supplicant<br/>builds dev_id"] LX["Linux — runtime supplicant<br/>builds dev_id"] OT["OP-TEE (this PR)<br/>key = HMAC(HUK, dev_id)"] HW["UFS RPMB Well-Known LUN"] UB -- "same 16-byte dev_id" --> OT LX -- "same 16-byte dev_id" --> OT OT --> HWU-Boot and Linux never call each other, but both must compute the byte-identical
dev_id. OP-TEE alone ever programs the key; the normal world only carries frames.Companion series (land together / keep in lockstep)
(
ufs: rpmb, v3):https://lkml.org/lkml/2026/8/21/1486
(UFS RPMB transport + OP-TEE subsystem supplicant, v4):
https://patchwork.ozlabs.org/project/uboot/list/?series=517522
Changing the algorithm, digest length, or input string in any one project is an
ABI break that orphans data already written to a provisioned RPMB.
NOTE:
I only have one Lemans board so I chose to provision using U-boot instead of Linux: so Linux provisioning is untested.
Test