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