[U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE
Jens Wiklander
jens.wiklander at linaro.org
Tue Nov 26 11:46:04 UTC 2019
On Tue, Nov 26, 2019 at 09:22:38AM +0100, Jorge Ramirez-Ortiz, Foundries wrote:
> 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 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.
>
> 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
On a little endian system cpu_to_be32() and be32_to_cpu() are both
implemented using uswap_32(). The only difference is in usage. With
cpu_to_be32() you have something in native byte order and convert it to
big endian, with be32_to_cpu() it's the other way around.
In this case (in rpmb_get_dev_info() far up in this conversion) we have
the 4 words in native endian and we need to convert them into big
endian.
Cheers,
Jens
More information about the U-Boot
mailing list