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 jorge@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 jorge@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
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?
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.
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