[U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE
Jorge Ramirez-Ortiz
jorge at foundries.io
Tue Nov 19 11:53:43 UTC 2019
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 <jorge at 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 at 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.
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
>>>>>>
>>>>
>>
More information about the U-Boot
mailing list