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.
>
> 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.
>
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
