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

[Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>]

On 20/11/19 11:33:10, Jens Wiklander wrote:
> On Wed, Nov 20, 2019 at 09:21:35AM +0100, Jorge Ramirez-Ortiz wrote:
> > On 11/20/19 8:20 AM, Jens Wiklander wrote:
> > > 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@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@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.

right, because the supplicant is implicitly doing the swap by picking
one byte at a time since the linux kernel wrote u32s and not bytes to
sysfs.so between them things balanced out.


> > 
> > which incidentally is exactly the same thing that linux does when the
> > MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.
> > 
> > static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
> > {
> > 	int ret, i;
> > 	__be32 *cid_tmp;
> > 
> > 	cid_tmp = kzalloc(16, GFP_KERNEL);
> > 	if (!cid_tmp)
> > 		return -ENOMEM;
> > 
> > 	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
> > 	if (ret)
> > 		goto err;
> > 
> > 	for (i = 0; i < 4; i++)
> > 		cid[i] = be32_to_cpu(cid_tmp[i]);
> > 
> > err:
> > 	kfree(cid_tmp);
> > 	return ret;
> > }
> > 
> > However, I think that cpu_to_be32() should be used
> > > instead for clarity. 
> > 
> > sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?
> 
> Yes, the words are in little endian but we need them to be in big endian
> when making it an array of u8.

no, sorry, I dont understand this. it would not work: we have to have
be32_to_cpu

really it is either the linux supplicant or uboot that have to
change. I supose uboot will be the preferred choice since it will have
less impact on current users.

> 
> > 
> > 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.
> > 
> > but why does the optee supplicant in uboot igonres the alignment request
> > made by optee (cmd_shm_alloc in uboot sets alignment to 0 while
> > thread_rpc_alloc_payload in optee requested 8)
> 
> Good question, looks like a bug to me. In this case it doesn't make much
> difference though since malloc() is required to return buffers which are
> at least 8 byte aligned.
> 
> > 
> > also any reason why we cant ask some other alignment from optee when
> > allocating the response buffer for the mmc dev info? then we dont have
> > to jump through hoops in uboot?
> 
> You mean the cache line aligned buffers? I don't see a problem with that,
> a patch is welcome. :-)
> 
> Any way, the suspicious cast which you're doing will cause a compiler
> warning with the right flags enabled. If you'd rather write a long
> comment explaining why it's perfectly safe to cast like that do so
> instead.
> 
> Cheers,
> Jens