On 26/11/19 12:46:04, Jens Wiklander wrote:
> 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
> > > > >>>>>>>> <jo...@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 <jo...@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.
yes, of course you are right (I double checked those implementations
in LE and I am with you now).
will repost the patch
thanks!
>
> Cheers,
> Jens
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
