Re: [PATCH v5 0/6] ufs: rpmb: route OP-TEE RPMB secure storage over UFS

[Jorge Ramirez <jorge.ramirez@oss.qualcomm.com>]

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 (林智斌)
> <Macpaul.Lin@mediatek.com> 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/20260716083728.2226422-1-jorge.ramirez@oss.qualcomm.com/
> > 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