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

[Jens Wiklander]

On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> > On 11/19/19 10:02 AM, Jens Wiklander wrote:
> >> On Mon, Nov 18, 2019 at 02:18:55PM +0100, Jorge Ramirez-Ortiz wrote:
> >>> On 11/18/19 1:42 PM, Jens Wiklander wrote:
> >>>> [+ 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.
> >>>
> >>> right, I dont know.
> >>>>
> >>>> 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.
> >>>
> >>> um my bad...let me clarify: when I was hacking around the issues I had
> >>> with the rpmb uboot driver, I was merging/testing some of the code from
> >>> the emulation mode in the linux tee-supplicant (rpbm values are
> >>> converted to network byte order); doing so allowed me to moved through
> >>> the response validation stage in optee so I figured that CID probably
> >>> was missing some sort of conversion as well.
> >>>
> >>>
> >>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> 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.
> >>>
> >>> ok.
> >>> if there is anything you'd like me to test or validate please let me know
> >>
> >> I'm not convinced that this is a generic problem. I don't doubt that
> >> it's a problem on the hardware you're using. Perhaps there's some
> >> byteswap missing in the driver for you hardware. So if you could figure
> >> out why the CID is in the wrong byte order with you're hardware it would
> >> help a lot. Or confirm that CID always is supposed to be stored in big
> >> endian in struct mmc and that eventual deviations from that is wrong.
> 
> 
> Yeah, actually it is: but perhaps should be fixed in the Linux
> supplicant instead.
> 
> Both u-boot and Linux do read the CID properly from MMC and they both
> hold the same values in four u32 variables so I can confirm that the MMC
> drivers for the imx do the right thing

Good

> 
> However in the trusted environment the situation is a bit different:
> 
> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> concatenated u32 values (not as an array of sixteen u8 values).
> 
> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> discarding the endianess.
> 
> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> therefore keeping the endiannes.
> 
> It is clear that at this point, the value that will reach the OPTEE's
> rpmb driver from linux will be different to the one from uboot.
> 
> So we could either fix it in u-boot's RPMB driver (with the patch I
> posted) or in the Linux supplicant in the  read_cid(..) function.
> 
> But one of the two has to change  not only for consistency but to enable
> both u-boot and Linux to access rpmb during the boot process on any
> endian systems.
> 
> what do you think? does this make sense?
> 

Thanks for digging into this, now the problem is clear to me. At the
Linux side I think the CID is received by secure world with the bytes in
the expected order. You're original patch fixes this by byte swapping
the words as needed. However, I think that cpu_to_be32() should be used
instead for clarity. Then there's the issue of alignment with the
casting you do. It works today due to how the function is called, but
the compiler can't guarantee that since the struct rpmb_dev_info only
contains u8:s so it's only byte aligned. You need to handle that inside
the function.

Cheers,
Jens