Re: [PATCH v3 3/5] ufs: derive the per-region RPMB CID and size for OP-TEE

On 29/07/26 17:40:25, David Lechner wrote:
> On 7/23/26 9:38 AM, Jorge Ramirez-Ortiz wrote:
> > OP-TEE computes its RPMB authentication key from a device identifier, so
> > U-Boot must hand it the exact same identifier the Linux kernel uses;
> > otherwise the derived key differs and OP-TEE cannot access RPMB secure
> > storage provisioned under Linux, and vice versa. The OP-TEE RPMB probe
> > also needs each region's size and reliable-write count to discover which
> > regions exist and how they may be written. Provide both so OP-TEE secure
> > storage stays interoperable across U-Boot and Linux on the same device.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > ---
> >  drivers/ufs/Kconfig    |   7 +-
> >  drivers/ufs/ufs-rpmb.c | 148 +++++++++++++++++++++++++++++++++++++++++
> >  include/ufs.h          |   6 ++
> >  3 files changed, 159 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ufs/Kconfig b/drivers/ufs/Kconfig
> > index 99160a0819b..d39fcda42dc 100644
> > --- a/drivers/ufs/Kconfig
> > +++ b/drivers/ufs/Kconfig
> > @@ -95,11 +95,14 @@ config UFS_TI_J721E
> >  config SUPPORT_UFS_RPMB
> >  	bool "Enable UFS RPMB (Replay Protected Memory Block) support"
> >  	depends on UFS && OPTEE && !SUPPORT_EMMC_RPMB
> > +	select BLAKE2
> >  	help
> >  	  Route OP-TEE RPMB requests to the UFS RPMB Well-Known LUN using
> >  	  SCSI SECURITY PROTOCOL IN/OUT commands. Required for OP-TEE secure
> >  	  storage (CFG_RPMB_FS) on UFS-based platforms that have no eMMC.
> > -	  The OP-TEE supplicant handles a single RPMB transport, so this is
> > -	  mutually exclusive with the eMMC RPMB supplicant (SUPPORT_EMMC_RPMB).
> > +	  BLAKE2 is used to derive the fixed-length RPMB CID that matches the
> > +	  Linux kernel UFS device_id ABI. The OP-TEE supplicant handles a
> > +	  single RPMB transport, so this is mutually exclusive with the eMMC
> > +	  RPMB supplicant (SUPPORT_EMMC_RPMB).
> >  
> >  endmenu
> > diff --git a/drivers/ufs/ufs-rpmb.c b/drivers/ufs/ufs-rpmb.c
> > index fb4cd42ad75..ed5ecc0a5e6 100644
> > --- a/drivers/ufs/ufs-rpmb.c
> > +++ b/drivers/ufs/ufs-rpmb.c
> > @@ -1,10 +1,14 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >  #include <dm.h>
> > +#include <hexdump.h>
> >  #include <log.h>
> >  #include <malloc.h>
> >  #include <scsi.h>
> >  #include <ufs.h>
> > +#include <vsprintf.h>
> > +#include <u-boot/blake2.h>
> 
> These are mostly in alphabetical order, so would make sense to put
> <u-boot/... after <linux/...

ok

> 
> >  #include <asm/cache.h>
> > +#include <asm/unaligned.h>
> >  #include <linux/errno.h>
> >  #include <linux/string.h>
> >  #include "ufs.h"
> > @@ -25,6 +29,15 @@
> >  
> >  #define GEOMETRY_DESC_RPMB_RW_SIZE	0x17
> >  
> > +#define RPMB_UNIT_DESC_LOGICAL_BLK_SIZE		0x0A
> > +#define RPMB_UNIT_DESC_LOGICAL_BLK_COUNT	0x0B
> > +#define RPMB_UNIT_DESC_REGION0_SIZE		0x13
> > +#define RPMB_UNIT_DESC_REGION1_SIZE		0x14
> > +#define RPMB_UNIT_DESC_REGION2_SIZE		0x15
> > +#define RPMB_UNIT_DESC_REGION3_SIZE		0x16
> > +#define UFS_RPMB_LEGACY_SPEC_VER		0x0220
> > +#define UFS_RPMB_REGION_UNIT_SHIFT		17
> > +
> >  static u16 rpmb_frame_request(const void *frame)
> >  {
> >  	const u8 *p = frame;
> > @@ -171,3 +184,138 @@ static int ufs_rpmb_read_geometry(struct udevice *scsi_dev, u8 *rpmb_rw_size)
> >  
> >  	return 0;
> >  }
> > +
> > +static int ufs_rpmb_read_region_sizes(struct ufs_hba *hba, u16 spec_ver,
> > +				      u8 sizes[UFS_RPMB_NUM_REGIONS])
> > +{
> > +	u8 unit[QUERY_DESC_UNIT_DEF_SIZE] = { };
> > +	int ret;
> > +
> > +	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT,
> > +				     UFS_UPIU_RPMB_WLUN, 0, unit, sizeof(unit));
> > +	if (ret)
> > +		return ret;
> > +
> > +	memset(sizes, 0, UFS_RPMB_NUM_REGIONS);
> > +
> > +	if (spec_ver > UFS_RPMB_LEGACY_SPEC_VER) {
> > +		sizes[0] = unit[RPMB_UNIT_DESC_REGION0_SIZE];
> > +		sizes[1] = unit[RPMB_UNIT_DESC_REGION1_SIZE];
> > +		sizes[2] = unit[RPMB_UNIT_DESC_REGION2_SIZE];
> > +		sizes[3] = unit[RPMB_UNIT_DESC_REGION3_SIZE];
> > +	} else {
> > +		u64 region = (get_unaligned_be64(unit +
> > +					RPMB_UNIT_DESC_LOGICAL_BLK_COUNT)
> > +			      << unit[RPMB_UNIT_DESC_LOGICAL_BLK_SIZE])
> > +			     >> UFS_RPMB_REGION_UNIT_SHIFT;
> 
> A bit hard to read. Maybe like this?

agreed, thanks!

> 
> 		u64 region;
> 
> 		region = get_unaligned_be64(unit + RPMB_UNIT_DESC_LOGICAL_BLK_COUNT);
> 		region <<= unit[RPMB_UNIT_DESC_LOGICAL_BLK_SIZE];
> 		region >>= UFS_RPMB_REGION_UNIT_SHIFT;
> 
> Also probably worth checking value of unit[RPMB_UNIT_DESC_LOGICAL_BLK_SIZE] before
> using it if comes from a potentially untrusted source.
> 
> > +
> > +		sizes[0] = region > 0xff ? 0xff : region;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void ufs_rpmb_string_to_ascii(const u8 *raw, char *out, size_t outsz)
> > +{
> > +	int nchars = ((int)raw[QUERY_DESC_LENGTH_OFFSET] - QUERY_DESC_HDR_SIZE);
> > +	int i, n = 0;
> > +
> > +	nchars = nchars > 0 ? nchars / 2 : 0;
> > +	for (i = 0; i < nchars && n < (int)outsz - 1; i++) {
> > +		u16 c = get_unaligned_be16(raw + QUERY_DESC_HDR_SIZE + i * 2);
> > +
> > +		out[n++] = (c >= 0x20 && c <= 0x7e) ? (char)c : ' ';
> > +	}
> > +	out[n] = '\0';
> > +}
> 
> This looks similar to ufshcd_remove_non_printable(). Should we try
> to share code more?

um, not sure we will be saving much.

The two functions do different jobs: ufshcd_remove_non_... is just a
one-line "clamp a byte to printable ASCII, else space", while
ufs_rpmb_string_to_ascii is a decode loop that happens to end each
iteration with that same one-line clamp. The only shareable part is that
trivial one-liner. Reusing it would mean un-static'ing the helper in
ufs-uclass.c and exposing it through a header just to save one line —
more coupling than it's worth IMO

>
> > +
> > +static int ufs_rpmb_build_cid(struct ufs_hba *hba, const u8 *dev_desc,
> > +			      unsigned int region, u8 *cid)
> > +{
> > +	char serial_hex[QUERY_DESC_MAX_SIZE * 2 + 1];
> > +	u16 manf_id, spec_ver, dev_ver, manf_date;
> > +	u8 serial[QUERY_DESC_MAX_SIZE] = { };
> > +	char idstr[QUERY_DESC_MAX_SIZE * 3];
> > +	char model[MAX_MODEL_LEN * 8];
> > +	u8 raw[QUERY_DESC_MAX_SIZE];
> > +	u8 blen;
> > +	int ret;
> > +
> > +	manf_date = get_unaligned_be16(dev_desc + DEVICE_DESC_PARAM_MANF_DATE);
> > +	spec_ver = get_unaligned_be16(dev_desc + DEVICE_DESC_PARAM_SPEC_VER);
> > +	manf_id = get_unaligned_be16(dev_desc + DEVICE_DESC_PARAM_MANF_ID);
> > +	dev_ver = get_unaligned_be16(dev_desc + DEVICE_DESC_PARAM_DEV_VER);
> > +
> > +	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_STRING,
> > +				     dev_desc[DEVICE_DESC_PARAM_PRDCT_NAME], 0,
> > +				     raw, sizeof(raw));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ufs_rpmb_string_to_ascii(raw, model, sizeof(model));
> > +
> > +	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_STRING,
> > +				     dev_desc[DEVICE_DESC_PARAM_SN], 0,
> > +				     raw, sizeof(raw));
> > +	if (ret)
> > +		return ret;
> > +
> > +	blen = raw[QUERY_DESC_LENGTH_OFFSET];
> > +	if (blen < QUERY_DESC_HDR_SIZE)
> > +		return -EINVAL;
> > +
> > +	memcpy(serial, raw + QUERY_DESC_HDR_SIZE, blen - QUERY_DESC_HDR_SIZE);
> > +	bin2hex(serial_hex, serial, blen);
> 
> Why isn't this also blen - QUERY_DESC_HDR_SIZE? Is the 00 padding intentional?

let me add a comment on the code, it is worth it.

> 
> > +	serial_hex[blen * 2] = '\0';
> > +
> > +	snprintf(idstr, sizeof(idstr), "%04X-%04X-%s-%s-%04X-%04X-R%u",
> > +		 manf_id, spec_ver, model, serial_hex, dev_ver, manf_date,
> > +		 region);
> > +
> > +	if (blake2b(cid, UFS_RPMB_CID_SIZE, idstr, strlen(idstr), NULL, 0))
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +