[U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE

[Jens Wiklander]

[+ Igor and Sam]

On Mon, Nov 18, 2019 at 12:18:27PM +0100, Jorge Ramirez-Ortiz wrote:
> On 11/18/19 10:36 AM, Jens Wiklander wrote:
> > Hi Jorge,
> 
> 
> hey!
> 
> >
> > On Fri, Nov 15, 2019 at 10:37 PM Jorge Ramirez-Ortiz <jorge at foundries.io> wrote:
> >> The MMC CID value is one of the input parameters to unequivocally
> >> provision the the RPMB key.
> >>
> >> Before this patch, the value returned by the mmc driver in the Linux
> >> kernel differs from the one returned by uboot to optee.
> >>
> >> This means that if Linux provisions the RPMB key, uboot wont be able
> >> to access it (and the other way around).
> >>
> >> Fix it so both uboot and linux can access the RPMB partition
> >> independently of who provisions the key.
> >>
> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge at foundries.io>
> >> ---
> >>  drivers/tee/optee/rpmb.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> >> index 955155b3f8..5dbb1eae4a 100644
> >> --- a/drivers/tee/optee/rpmb.c
> >> +++ b/drivers/tee/optee/rpmb.c
> >> @@ -98,6 +98,7 @@ static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
> >>  static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
> >>  {
> >>         struct mmc *mmc = find_mmc_device(dev_id);
> >> +       int i;
> >>
> >>         if (!mmc)
> >>                 return TEE_ERROR_ITEM_NOT_FOUND;
> >> @@ -105,7 +106,9 @@ static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
> >>         if (!mmc->ext_csd)
> >>                 return TEE_ERROR_GENERIC;
> >>
> >> -       memcpy(info->cid, mmc->cid, sizeof(info->cid));
> >> +       for (i = 0; i < ARRAY_SIZE(mmc->cid); i++)
> >> +               ((u32 *) info->cid)[i] = be32_to_cpu(mmc->cid[i]);
> >> +
> > So it seems to be a byte order issue. I can't find the place in the
> > Linux kernel (or in tee-supplicant) where the corresponding byte
> > swapping is done. Have you been able to find it or you just tried to
> > swap the bytes and it seemed to work?
> 
> 
> I compared against the full CID output from Linux and noticed that in
> order to match that exact same output this swap seemed to be required. I
> didnt dig any deeper since a similar swap operation is done on other
> -different - values returned from U-boot to OP-TEE.

So we don't know if the byte swap is always needed, only on little
endian machines or perhaps only with certain devices.

By the way, where are the other byte swaps you're mentioning? I did a
quick grep under drivers/tee/ and didn't find anything.

> 
> 
> >
> > I'm not yet convinced that be32_to_cpu() is the correct function here.
> > OP-TEE masks out a few fields from the CID when deriving the key:
> 
> 
> sure but isnt that a different matter?

No, it's important that OP-TEE masks out the correct fields. That's why
we must make sure to understand the problem so we don't just push the
problem around.

> 
> AFAICS U-boot should be providing the same CID than Linux does, and
> whatever OP-TEE might be masking out on the receiving end is orthogonal
> to such value, isnt it? maybe I am not understanding your point?

I agree that something must be done so it works with Linux. However, I'm
a bit surprised that we haven't seen this earlier.

If there's an error in how it's done in Linux we may need to implement
some workaround in tee-supplicant or perhaps in secure world. If we wait
with that until after we have some workarounds in U-Boot too, stuff will
become even more messy.

Cheers,
Jens

> 
> 
> >
> > CID is a uint8_t[16] here
> >         /*
> >          * PRV/CRC would be changed when doing eMMC FFU
> >          * The following fields should be masked off when deriving RPMB key
> >          *
> >          * CID [55: 48]: PRV (Product revision)
> >          * CID [07: 01]: CRC (CRC7 checksum)
> >          * CID [00]: not used
> >          */
> >
> > Will this work as expected on a big endian machine?
> >
> > Cheers,
> > Jens
> >
> >>         info->rel_wr_sec_c = mmc->ext_csd[222];
> >>         info->rpmb_size_mult = mmc->ext_csd[168];
> >>         info->ret_code = RPMB_CMD_GET_DEV_INFO_RET_OK;
> >> --
> >> 2.23.0
> >>
>