From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jorge Ramirez-Ortiz Date: Tue, 19 Nov 2019 12:53:43 +0100 Subject: [U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE In-Reply-To: <20191119090223.GA32154@jax> References: <20191115213735.25711-1-jorge@foundries.io> <20191118124209.GA25796@jax> <762945ff-387b-edab-705f-106f200ac721@foundries.io> <20191119090223.GA32154@jax> Message-ID: <9f201f03-52b1-7664-9272-3e4a5b23ddb8@foundries.io> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. I had a quick look at the linux driver and both are using LE operations when reading from the corresponding registers. Moreover, when interpreting the CID response (ie RSP_136) they both perform the same arithmetics: uboot: fsl_esdhc_imx.c ------ if (cmd->resp_type & MMC_RSP_136) { u32 cmdrsp3, cmdrsp2, cmdrsp1, cmdrsp0; cmdrsp3 = esdhc_read32(®s->cmdrsp3); cmdrsp2 = esdhc_read32(®s->cmdrsp2); cmdrsp1 = esdhc_read32(®s->cmdrsp1); cmdrsp0 = esdhc_read32(®s->cmdrsp0); cmd->response[0] = (cmdrsp3 << 8) | (cmdrsp2 >> 24); cmd->response[1] = (cmdrsp2 << 8) | (cmdrsp1 >> 24); cmd->response[2] = (cmdrsp1 << 8) | (cmdrsp0 >> 24); cmd->response[3] = (cmdrsp0 << 8); } linux: ------ static void sdhci_read_rsp_136(struct sdhci_host *host, struct mmc_command *cmd) { int i, reg; for (i = 0; i < 4; i++) { reg = SDHCI_RESPONSE + (3 - i) * 4; cmd->resp[i] = sdhci_readl(host, reg); } if (host->quirks2 & SDHCI_QUIRK2_RSP_136_HAS_CRC) return; /* CRC is stripped so we need to do some shifting */ for (i = 0; i < 4; i++) { cmd->resp[i] <<= 8; if (i != 3) cmd->resp[i] |= cmd->resp[i + 1] >> 24; } } so it seems to me that both drivers are doing the same thing. I'll try to have another look towards the end of the week, maybe adding some extra debug. > >> >>> >>>> >>>> AFAICS U-boot should be providing the same CID than Linux does, and >>>> whatever OP-TEE might be masking out on the receiving end is orthogonal >>>> to such value, isnt it? maybe I am not understanding your point? >>> >>> I agree that something must be done so it works with Linux. However, I'm >>> a bit surprised that we haven't seen this earlier. >> >> could be that accessing rpmb has never been done from both linux and >> u-boot? > > I'm sure I've tested that on Hikey when I implemented this stuff. I know > I wrote the key using Linux since I didn't have the complete chain in > U-Boot to start with then. um ok. hikey is also little endian (in byteorder.h) > >> >> in fact when I was trying to access rpmb values from uboot via AVB I >> also noticed that the current code (at least in my imx7 platform) >> wouldnt work due to cache alignment issues...so needed an additional >> patch (which I still need to send to this ML) to use aligned buffers on >> the stack in the read/write rpmb functions. >> >>> >>> If there's an error in how it's done in Linux we may need to implement >>> some workaround in tee-supplicant or perhaps in secure world. If we wait >>> with that until after we have some workarounds in U-Boot too, stuff will >>> become even more messy. yes I understand your point. ok. >>> >>> Cheers, >>> Jens >>> >>>> >>>> >>>>> >>>>> CID is a uint8_t[16] here >>>>> /* >>>>> * PRV/CRC would be changed when doing eMMC FFU >>>>> * The following fields should be masked off when deriving RPMB key >>>>> * >>>>> * CID [55: 48]: PRV (Product revision) >>>>> * CID [07: 01]: CRC (CRC7 checksum) >>>>> * CID [00]: not used >>>>> */ >>>>> >>>>> Will this work as expected on a big endian machine? >>>>> >>>>> Cheers, >>>>> Jens >>>>> >>>>>> info->rel_wr_sec_c = mmc->ext_csd[222]; >>>>>> info->rpmb_size_mult = mmc->ext_csd[168]; >>>>>> info->ret_code = RPMB_CMD_GET_DEV_INFO_RET_OK; >>>>>> -- >>>>>> 2.23.0 >>>>>> >>>> >>