On 25/09/26 12:59:28, Ilias Apalodimas wrote: > Thanks for the detailed analysis. > > I am pulling in Jens for OP-TEE and I'll have a closer look next week as well.
the concernt I read - please correct me if I am wrong - is that this code affects downstream projects. This is just not acceptable to me - downstream implementations should have been posted to prevent exactly these kind issues. We spent time and effort doing an implementation and sharing it with the community - it wouldnt be fair to us to block this merge on the basis of downstream projects being impacted. > > /Ilias > > On Thu, 24 Sept 2026 at 20:50, Macpaul Lin (林智斌) > <[email protected]> wrote: > > > > On 18/09/26 22:06:32, Jorge Ramirez-Ortiz wrote: > > > OP-TEE secure storage (CFG_RPMB_FS) relies on an RPMB partition, but > > > U-Boot's OP-TEE RPMB supplicant only speaks the legacy single-command > > > interface, which is bound to eMMC. SoCs that are UFS-only and have no > > > eMMC (for example the Qualcomm SA8775P) therefore cannot back OP-TEE > > > secure storage from U-Boot today. This series adds that support. > > > > > > It introduces the transport-agnostic OP-TEE RPMB "subsystem" interface > > > (PROBE_RESET / PROBE_NEXT / FRAMES), where the normal world enumerates > > > the RPMB device and reports its kind, size and CID, then carries the > > > signed frames. The legacy eMMC supplicant is preserved unchanged, only > > > renamed to rpmb_emmc.c, with the new UFS backend added as a separate > > > rpmb_ufs.c; the two are mutually exclusive via Kconfig (SUPPORT_UFS_RPMB > > > depends on !SUPPORT_EMMC_RPMB) because the OP-TEE supplicant handles a > > > single RPMB transport. The subsystem interface is UFS-only for now; eMMC > > > can be migrated onto it later as the legacy path is retired. > > > > > > On top of that it adds a UFS RPMB transport that moves JEDEC RPMB frames > > > to and from the RPMB Well-Known LUN using SCSI SECURITY PROTOCOL IN/OUT. > > > The per-region 16-byte CID is derived by BLAKE2b-hashing the exact > > > device-id string the Linux kernel builds (ufshcd_create_device_id() > > > plus a "-R<region>" suffix), so OP-TEE derives an RPMB key that matches > > > the one Linux would use. > > > > > > The first patch is a standalone UFS descriptor fix the RPMB path depends > > > on (UTF-16BE string decoding). A small preparatory patch surfaces the > > > RESPONSE UPIU sense data from ufs_scsi_exec() so the transport can retry > > > only on a power-on UNIT ATTENTION; the transport patch also bounces > > > unaligned frames through a DMA-aligned buffer for the RPMB WLUN. > > > > > > Note: reading UFS descriptors reliably also requires the descriptor > > > data-segment cache-invalidation fix, which has already been posted and > > > merged separately, so this series is based on top of it. > > > > > > Tested on the Qualcomm IQ-9075-EVK (SA8775P): OP-TEE with CFG_RPMB_FS > > > programs the RPMB key through U-Boot and reads/writes secure-storage > > > objects, with the derived CID matching the Linux UFS device_id ABI. > > > > > > Dependencies: > > > Linux kernel: > > > > > > https://lore.kernel.org/linux-scsi/[email protected]/ > > accepted in v4 --> https://lkml.org/lkml/2026/8/27/369 > > > > > Op-tee > > > > > > https://urldefense.com/v3/__https://github.com/OP-TEE/optee_os/pull/7881__;!!CTRNKA9wMg0ARbw!lQ5D9IUE1D5erqbaXCklFua6JiijZYB6XN0q5LDFZZpiaGmrH_k10rLkcjU1nSzhsRDX--J49Gt22E8U73lx7Es567TUAxwX$ > > and also following Ilias review > > https://urldefense.com/v3/__https://github.com/OP-TEE/optee_os/pull/8007__;!!CTRNKA9wMg0ARbw!lQ5D9IUE1D5erqbaXCklFua6JiijZYB6XN0q5LDFZZpiaGmrH_k10rLkcjU1nSzhsRDX--J49Gt22E8U73lx7Es565JETGdQ$ > > (both merged) > > > > > For reference, at this point Linux can access RPMB and just u-boot > > > pending on this set. > > I'd like to have some discussion in more detail as below, it will be quite > > long. However, my company's > > email server and mail clients both have problem again as usual. I'm not > > sure if this reply could be send > > out correctly and in correct format. Let's see. > > Moreover, I've invited Kernel's maintainers Stanley Jhu and Bean Huo to > > this mail thread to see if the opinion increases more compatibility. > > But, I'm not sure how to invite OPTEE's maintainers, I'll try to forward > > the following description to OPTEE later. > > > > So.. let's start from patch 4/6 and then review the whole story. > > 4/6 — ufs: derive the per-region RPMB CID and size for OP-TEE > > > > Patch 4/6 hard-codes both the hash algorithm (BLAKE2b) and the output > > length (UFS_RPMB_CID_SIZE = 16) used to derive the RPMB CID for UFS > > devices. Neither choice is mandated by any specification the series > > builds on (UFS/JESD220, RPMB, or the OP-TEE/TEE Client API), and > > hard-coding them creates a real compatibility risk for products that > > have already shipped with a different, vendor-specific derivation. The > > CID derivation should be factored out as a vendor-specific, build-time > > selectable component rather than a single fixed algorithm baked into > > common UFS core code. I am a bit at lost. Why didnt those products pushed their code upstream? If they have an issue with this proposal, they can apply their needs on top, can they not? > > > > 1. Neither the UFS, RPMB, nor TEE specifications mandate a hash > > algorithm for CID derivation > > - The UFS specification (JESD220) defines the Device/Unit/Geometry > > descriptors that patch 4/6 reads (manufacturer ID, spec version, > > model string, serial number, device version, manufacture date, > > region sizes) — it says nothing about combining them into a "CID", > > and certainly nothing about which hash function to use. > > - The RPMB frame protocol itself (struct rpmb_frame in > > include/linux/rpmb.h, and the equivalent JEDEC RPMB frame layout) > > has no CID field at all — CID is purely a host-side/secure-world > > key management convention, not part of the wire protocol. > > - The GlobalPlatform TEE Client API / OP-TEE's own RPMB subsystem RPC > > contract only defines that a fixed-size opaque byte buffer is passed > > as the device identifier — it does not specify how that buffer's > > contents are derived from the underlying storage device. > > - Because no specification prescribes the algorithm, this is > > legitimately vendor-specific territory, and different vendors — or > > even the same vendor across product generations — may reasonably > > choose different KDFs/hash functions (BLAKE2b, SHA-256, a > > vendor-proprietary hash, or a raw non-hashed encoding, as seen in > > other real-world implementations of this same feature). The patch > > set should not force one specific vendor's choice into common UFS > > core code as the only option. > > > > 2. UFS RPMB does not mandate a 16-byte CID; the size should be variable > > > > - The 16-byte length is not derived from any UFS or RPMB requirement. > > It comes from the eMMC CID register (JEDEC JESD84 Card/Device > > Identification Register), which is a fixed 128-bit hardware > > register — a purely eMMC-specific hardware fact. > > - Linux kernel's own generic RPMB abstraction (struct rpmb_descr in > > include/linux/rpmb.h) already models this correctly and treats the > > device identifier as variable-length by design: > > > > struct rpmb_descr { > > enum rpmb_type type; > > int (*route_frames)(struct device *dev, u8 *req, unsigned int > > req_len, > > u8 *resp, unsigned int resp_len); > > u8 *dev_id; > > size_t dev_id_len; /* variable, not fixed at 16 */ > > u16 reliable_wr_count; > > u16 capacity; > > }; > > > > Likewise, the UFS-specific device identifier the Linux kernel builds > > in ufshcd_create_device_id() is a variable-length ASCII string > > ("%04X-%04X-%s-%s-%04X-%04X"), not a fixed 16-byte value. > > - Patch 4/6 should follow the same pattern: UFS_RPMB_CID_SIZE should > > not be a hard #define 16 baked into include/ufs.h; the CID buffer > > size should be a property of the selected vendor backend (e.g. > > returned alongside the derived CID, or declared per-backend), so > > that a 16-byte output is one valid choice among others rather than > > an assumed universal constant. > > > > 3. The vendor-specific part should be factored out; 16 bytes is only > > there for eMMC compatibility, and UFS has no such hardware CID at all > > > > - To restate the core problem plainly: UFS has no CID register. > > There is no piece of UFS hardware that patch 4/6's > > ufs_rpmb_build_cid() is "reading" — it is entirely a software > > construction. The only reason the result is squeezed into 16 bytes > > is to fit the pre-existing OP-TEE RPC buffer size that was > > originally designed around eMMC's real, hardware-defined 16-byte CID > > register. > > - Conflating "the UFS-specific descriptor-reading/string-building > > logic" with "the specific hash function and output length choice" > > in a single non-configurable function means every downstream user of > > this series inherits one vendor's algorithm choice with no way to > > opt out, even though the two concerns are logically separable: > > - Reading UFS descriptors and building the input identifier string > > is common, spec-driven logic that can stay in shared UFS core > > code. > > - Turning that string into the actual CID bytes handed to OP-TEE is > > the vendor-specific part and should be pluggable. why cant you extend what is being proposed in this series? > > - Suggested restructuring: keep ufshcd-side descriptor reads and > > string-building in common code, but expose the final > > string-to-CID step as a replaceable hook/weak function (or a small > > vendor ops table), so the BLAKE2b implementation in this series > > becomes the default backend rather than the only backend. sure, send a patch on top of this one. > > > > 4. Hard-coding one algorithm creates real firmware-upgrade and > > key-compatibility risk for already-shipping products, so the > > derivation must be vendor-selectable at build time (via defconfig) I dont see why since U-boot currently does not support UFS RPMB. IF downstream has an issue, please send patches. > > > > 5. Since neither the algorithm (point 1) nor the output length (point 2) > > is specified by any standard, and CID is purely a software > > convention invented to reuse an eMMC-shaped OP-TEE ABI (point 3), > > different implementations that already exist today have made > > different, incompatible choices for the same problem: I dont see this code upstream. How can this be a blocker? > > - This series (upstream v5, patch 4/6): builds an identifier string > > from Device Descriptor fields (manufacturer ID, spec version, > > model, serial, device version, manufacture date) and hashes it > > with BLAKE2b to a fixed 16-byte output. > > - At least one other real, shipping downstream implementation (a > > vendor's production U-Boot branch analyzed alongside this series) > > derives the CID by directly decoding/porting the device's > > tee-supplicant-side read_cid()-style logic — i.e. no hash at all, > > just a direct byte encoding of the serial-number string > > descriptor. You keep on mentioning an impact to downstream and my reasoning is the same: why didnt they push their implementations upstream? > > - Both approaches are equally "valid" in the sense that neither > > violates any specification, because none exists to violate — but > > they are mutually incompatible: the same physical UFS device will > > produce a different 16-byte value under each scheme, and > > therefore OP-TEE will derive a different final RPMB authentication > > key under each scheme. > > - The practical consequence: any product that has already shipped in > > the field with one CID-derivation scheme (whether that is this > > series's BLAKE2b approach or a vendor's own pre-existing scheme) has > > effectively burned that scheme into its provisioned RPMB key > > material. If a firmware update later replaces the bootloader/OP-TEE > > RPMB glue with a build that hard-codes a different CID derivation > > (for example, adopting this patch set's BLAKE2b implementation > > unconditionally on top of a product that shipped with a different > > vendor scheme), the newly-derived CID will not match the CID used > > when the device's RPMB key material was originally provisioned. The > > device will then either: > > - fail to authenticate against its own RPMB partition (OP-TEE > > derives a key that doesn't match what was provisioned), or > > - silently begin deriving a different key than before, which is > > just as bad if any secure-storage data was provisioned under the > > old key and must remain readable after the upgrade. > > > > Because CID derivation happens deep inside firmware/secure-world > > boot code with no user-visible error surface until the RPMB HMAC > > check actually fails, this class of bug is essentially undetectable > > in normal testing and only surfaces as field failures after an > > upgrade — exactly the "silent, undetectable until it fails" failure > > mode already called out in the CID appendix. > > - Therefore, the fix is not merely a documentation/comment change: > > the patch set should ship this concern as a build-time (Kconfig / > > defconfig) selectable option, e.g.: > > > > config UFS_RPMB_CID_ALGO_BLAKE2B > > string "UFS RPMB CID derivation algorithm" > > depends on SUPPORT_UFS_RPMB > > default "blake2b" > > help > > Selects which algorithm derives the per-region RPMB CID > > from UFS device descriptors. This value is a software > > convention with no basis in the UFS or RPMB specification > > (UFS has no hardware CID register); it exists only to > > populate the fixed-size CID field inherited from the > > eMMC-oriented OP-TEE RPMB ABI. Products that have already > > shipped with a specific CID derivation MUST keep using > > that same derivation across firmware updates, or the > > derived RPMB authentication key will change and existing > > RPMB-backed secure storage will become unreadable. > > > > with the BLAKE2b implementation in this series wired in as one > > selectable backend (CONFIG_UFS_RPMB_CID_ALGO_BLAKE2B), so that a > > vendor already shipping a different scheme can select their own > > backend at defconfig time instead of being forced onto this > > series's default and breaking already-provisioned devices on > > upgrade. > > > > Each vendor should be able to upstream their own algo and more. I dont see why but feel free to propose a patch on top of thise series. > > And these algo should be configurable by the users or by board makers. > > For example, MediaTek and Ubuntu are using different algo and CID length for > > UFS RPMB. > > > > Here we suggest to make some change to increase the compatibility. > > > > 1. Split ufs_rpmb_build_cid() into (a) a common helper that reads the > > UFS descriptors and builds the identifier string, and (b) a > > pluggable string-to-CID backend, with BLAKE2b as the default > > backend rather than the only one. That would break kernel compatibility - the kernel uses blake2b. > > 2. Make the CID buffer size a variable size, or at least property returned > > by the selected > > backend rather than a fixed UFS_RPMB_CID_SIZE = 16 constant in > > common code. What would be the benefit of this? > > 3. Add a Kconfig choice (e.g. UFS_RPMB_CID_ALGO_BLAKE2) so vendors can > > select their existing/legacy CID derivation at defconfig time. And the kernel? how are you going to handled that? This would be a source of pain when doing software upgrades. > > 4. Document explicitly, in code comments and Kconfig help text, that > > this value is a software ABI convention with no basis in the UFS > > or RPMB specification, and that changing it on an already-shipped > > product is a breaking change to that product's RPMB key material. > > > > Thanks > > Macpaul Lin
