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

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/...

>  #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?

		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?

> +
> +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?

> +	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;
> +}
> +