From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jorge Ramirez-Ortiz, Foundries Date: Tue, 26 Nov 2019 16:41:39 +0100 Subject: [U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE In-Reply-To: <20191126114602.GA9641@jax> References: <20191118124209.GA25796@jax> <762945ff-387b-edab-705f-106f200ac721@foundries.io> <20191119090223.GA32154@jax> <9f201f03-52b1-7664-9272-3e4a5b23ddb8@foundries.io> <8666fb53-91c5-b700-6f71-dd8f93fdd620@foundries.io> <20191120071959.GA4878@jax> <20191120103308.GA10259@jax> <20191126082238.GA9538@trex> <20191126114602.GA9641@jax> Message-ID: <20191126154139.GA10010@trex> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 > > > > >>>>>>>>> --- > > > > >>>>>>>>> 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