From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jens Wiklander Date: Mon, 18 Nov 2019 13:42:10 +0100 Subject: [U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE In-Reply-To: References: <20191115213735.25711-1-jorge@foundries.io> Message-ID: <20191118124209.GA25796@jax> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de [+ 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. 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. > > > > > > 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. > > 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. 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. 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 > >> >