Re: [PATCH v1 2/2] ufs: rpmb: use a fixed-length RPMB dev_id

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

On 17/07/26 11:38:31, Jens Wiklander wrote:
> Hi Jorge,
> 
> On Thu, Jul 16, 2026 at 10:37 AM Jorge Ramirez-Ortiz
> <jorge.ramirez@oss.qualcomm.com> wrote:
> >
> > The RPMB authentication key is derived from the dev_id handed to the
> > RPMB subsystem. OP-TEE implements the eMMC RPMB flow, where the dev_id
> > is the eMMC CID, a fixed 16-byte value, and it derives the key on that
> > assumption.
> >
> > The UFS RPMB id built here is "<device_id>-R<region>", which is variable
> > length and longer than 16 bytes. Passing it verbatim would tie the
> > derived key to a length OP-TEE does not expect and diverge from the
> > fixed-CID eMMC ABI, requiring OP-TEE to be taught about variable-length
> > UFS ids.
> 
> Yes, if it's possible, It's nice to avoid that.
> 
> >
> > Hash the UFS id into a fixed 16-byte dev_id with blake2s instead. This
> > keeps the derived key stable and unique per region while matching the
> > eMMC CID layout OP-TEE relies on, so the key-derivation ABI stays
> > identical and no OP-TEE change is needed.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> > ---
> >  drivers/ufs/core/ufs-rpmb.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> > index d0c7ea7a36f4..b800871269bb 100644
> > --- a/drivers/ufs/core/ufs-rpmb.c
> > +++ b/drivers/ufs/core/ufs-rpmb.c
> > @@ -10,6 +10,7 @@
> >   *     Can Guo <can.guo@oss.qualcomm.com>
> >   */
> >
> > +#include <crypto/blake2s.h>
> >  #include <linux/module.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> > @@ -21,6 +22,7 @@
> >  #include <linux/unaligned.h>
> >  #include "ufshcd-priv.h"
> >
> > +#define UFS_RPMB_ID_LEN                        16      /* Match eMMC CID Length */
> >  #define UFS_RPMB_SEC_PROTOCOL          0xEC    /* JEDEC UFS application */
> >  #define UFS_RPMB_SEC_PROTOCOL_ID       0x01    /* JEDEC UFS RPMB protocol ID, CDB byte3 */
> >
> > @@ -154,6 +156,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >  {
> >         struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
> >         struct rpmb_dev *rdev;
> > +       char *dev_id = NULL;
> >         char *cid = NULL;
> >         int region;
> >         u32 cap;
> > @@ -213,8 +216,17 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >                         goto err_out;
> >                 }
> >
> > -               descr.dev_id = cid;
> > -               descr.dev_id_len = strlen(cid);
> > +               dev_id = kzalloc(UFS_RPMB_ID_LEN, GFP_KERNEL);
> > +               if (!dev_id) {
> > +                       device_unregister(&ufs_rpmb->dev);
> > +                       ret = -ENOMEM;
> > +                       goto err_out;
> > +               }
> > +
> > +               blake2s(NULL, 0, cid, strlen(cid), dev_id, UFS_RPMB_ID_LEN);
> > +
> > +               descr.dev_id = dev_id;
> > +               descr.dev_id_len = UFS_RPMB_ID_LEN;
> 
> This change will break current users of this interface, if there are
> any. There are currently no upstream users in OP-TEE since you're
> adding that with https://github.com/OP-TEE/optee_os/pull/7881, but I
> suppose that's not the only use case.


I was wondering too - but since this is an OP-TEE driver (at least that
is what the module directive says at the bottom of the file) - I chose
to do it here (cleaner).

But we could take care of this on the rpc.c instead

A similar sort of change will need to go to U-boot but since the current
CID is broken anyway, I believe we will be able to go that route as
well.

See: https://patchwork.ozlabs.org/project/uboot/patch/20260717093013.3253643-1-jorge.ramirez@oss.qualcomm.com/

BTW I need to send another revision of this set (we need to use blake2b
(there is no support for blake2s in U-boot). But I'll wait for comments
if this is the right way to go instead of using full-size srings (ie, 88
characters on this particular case which seems desproportionate IMO)

> 
> Cheers,
> Jens
> 
> 
> >                 descr.capacity = cap;
> >
> >                 /* Register RPMB device */
> > @@ -228,6 +240,8 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >
> >                 kfree(cid);
> >                 cid = NULL;
> > +               kfree(dev_id);
> > +               dev_id = NULL;
> >
> >                 ufs_rpmb->rdev = rdev;
> >                 ufs_rpmb->region_id = region;
> > @@ -240,6 +254,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >         return 0;
> >  err_out:
> >         kfree(cid);
> > +       kfree(dev_id);
> >         list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
> >                 list_del(&it->node);
> >                 device_unregister(&it->dev);
> > --
> > 2.54.0
> >