On 10/09/26 10:55:47, Ilias Apalodimas wrote:
> Hi Jorge,
>
> [...]
>
> > +
> > +void optee_suppl_cmd_rpmb_probe_next(struct udevice *dev,
> > + struct optee_msg_arg *arg)
> > +{
> > + struct optee_private *priv = dev_get_priv(dev);
> > + struct udevice *scsi_dev;
> > + struct tee_shm *cid_shm;
> > + u8 size_mult = 0;
> > + u8 rel_wr = 0;
> > + void *cid_buf;
> > + ulong cid_size;
> > + int ret;
> > +
> > + if (arg->num_params != 2 ||
> > + arg->params[0].attr != OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT ||
> > + arg->params[1].attr != OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT) {
> > + arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > + return;
> > + }
> > +
> > + cid_shm = (struct tee_shm *)(ulong)arg->params[1].u.rmem.shm_ref;
> > + cid_buf = (u8 *)cid_shm->addr + arg->params[1].u.rmem.offs;
> > + cid_size = arg->params[1].u.rmem.size;
> > + if (cid_size < UFS_RPMB_CID_SIZE) {
> > + arg->ret = TEE_ERROR_SHORT_BUFFER;
> > + return;
> > + }
> > +
> > + if (optee_rpmb_get_dev(&scsi_dev)) {
> > + arg->ret = TEE_ERROR_ITEM_NOT_FOUND;
> > + return;
> > + }
> > +
> > + while (priv->rpmb_next_region < UFS_RPMB_NUM_REGIONS) {
> > + unsigned int region = priv->rpmb_next_region++;
> > +
>
> We had a brief chat offline, but here's what I think is happening.
>
> OP-TEE and U-Boot have their own view of 'partitions to write'.
> What I think is happening here if the UFS RPMB partitions is > 1 is
> - For OP-TEE have_cand is only set once, for the first partition and restored
> just before writing
> the key
> - U-Boot sets rpmb_cur_region independantly and exits whenever it exhausts
> all the RPMB partitions
> - OP-TEE will restore the first key and probably write it to the wrong slot
>
> I haven't checked OP-TEE enough, but since this is an OTP I think we either
> need a way to verify we
> are writing the right key to the right slot, or the partition number actually
> calculated and
> instructed by OP-TEE (in the TA arguments perhaps?)
>
> I really prefer the latter, but I understand it's a much bigger change, plus
> the OP-TEE code is
> already merged. So there's always the chance that someone is unlucky enough
> to checkout a wrong
> combination of U-Boot/OP-TEE and 'brick' his RPMB.
>
> It apparently didn't show up on any testing, because it works fine for a
> single partition.
thanks Ilias - you are 100% right.
A last minute change went in op-tee to scan all available RPMBs
partitions and restore a context which wouldnt move the u-boot
index.
we can address it in op-tee via CFG_ (simple, the config already exists,
we just apply the write instead of postponing it); and in parallel
develop a more user friendly approach as the one you described. I think
we all agree are better solution is required.
yeah it didnt show up on testing as I only had one RPMB device....my bad
>
> Cheers
> /Ilias
>
> > + ret = ufs_rpmb_get_region_info(scsi_dev, region, &size_mult,
> > + &rel_wr, cid_buf);
> > + if (ret < 0) {
> > + arg->ret = TEE_ERROR_GENERIC;
> > + return;
> > + }
> > + if (!ret)
> > + continue;
> > +
> > + priv->rpmb_cur_region = region;
> > + arg->params[0].u.value.a = OPTEE_RPC_RPMB_UFS;
> > + arg->params[0].u.value.b = size_mult;
> > + arg->params[0].u.value.c = rel_wr;
> > + arg->params[1].u.rmem.size = UFS_RPMB_CID_SIZE;
> > + arg->ret = TEE_SUCCESS;
> > + return;
> > + }
> > +
> > + arg->ret = TEE_ERROR_ITEM_NOT_FOUND;
> > +}
> > +
> > +void optee_suppl_cmd_rpmb_frames(struct udevice *dev,
> > + struct optee_msg_arg *arg)
> > +{
> > + struct optee_private *priv = dev_get_priv(dev);
> > + struct tee_shm *req_shm;
> > + struct tee_shm *rsp_shm;
> > + struct udevice *scsi_dev;
> > + void *req_buf;
> > + void *rsp_buf;
> > + ulong req_size;
> > + ulong rsp_size;
> > +
> > + if (arg->num_params != 2 ||
> > + arg->params[0].attr != OPTEE_MSG_ATTR_TYPE_RMEM_INPUT ||
> > + arg->params[1].attr != OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT) {
> > + arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > + return;
> > + }
> > +
> > + if (optee_rpmb_get_dev(&scsi_dev)) {
> > + arg->ret = TEE_ERROR_ITEM_NOT_FOUND;
> > + return;
> > + }
> > +
> > + req_shm = (struct tee_shm *)(ulong)arg->params[0].u.rmem.shm_ref;
> > + req_buf = (u8 *)req_shm->addr + arg->params[0].u.rmem.offs;
> > + req_size = arg->params[0].u.rmem.size;
> > +
> > + rsp_shm = (struct tee_shm *)(ulong)arg->params[1].u.rmem.shm_ref;
> > + rsp_buf = (u8 *)rsp_shm->addr + arg->params[1].u.rmem.offs;
> > + rsp_size = arg->params[1].u.rmem.size;
> > +
> > + if (ufs_rpmb_route_frames(scsi_dev, priv->rpmb_cur_region, req_buf,
> > + req_size, rsp_buf, rsp_size))
> > + arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > + else
> > + arg->ret = TEE_SUCCESS;
> > +}
> > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > index 8a426f53ba8..50b780037fb 100644
> > --- a/drivers/tee/optee/supplicant.c
> > +++ b/drivers/tee/optee/supplicant.c
> > @@ -89,6 +89,15 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm
> > *shm_arg,
> > case OPTEE_MSG_RPC_CMD_RPMB:
> > optee_suppl_cmd_rpmb(dev, arg);
> > break;
> > + case OPTEE_MSG_RPC_CMD_RPMB_PROBE_RESET:
> > + optee_suppl_cmd_rpmb_probe_reset(dev, arg);
> > + break;
> > + case OPTEE_MSG_RPC_CMD_RPMB_PROBE_NEXT:
> > + optee_suppl_cmd_rpmb_probe_next(dev, arg);
> > + break;
> > + case OPTEE_MSG_RPC_CMD_RPMB_FRAMES:
> > + optee_suppl_cmd_rpmb_frames(dev, arg);
> > + break;
> > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> > optee_suppl_cmd_i2c_transfer(arg);
> > break;
> > diff --git a/drivers/ufs/Kconfig b/drivers/ufs/Kconfig
> > index d39fcda42dc..d7fe2486ee2 100644
> > --- a/drivers/ufs/Kconfig
> > +++ b/drivers/ufs/Kconfig
> > @@ -105,4 +105,12 @@ config SUPPORT_UFS_RPMB
> > single RPMB transport, so this is mutually exclusive with the eMMC
> > RPMB supplicant (SUPPORT_EMMC_RPMB).
> >
> > +config UFS_RPMB_CONTROLLER
> > + int "UFS controller index used for RPMB"
> > + depends on SUPPORT_UFS_RPMB
> > + default 0
> > + help
> > + Index of the RPMB-owning UFS controller in the UCLASS_UFS device
> > + list. Leave at 0 unless the board has more than one UFS controller.
> > +
> > endmenu
>
