[U-Boot] [PATCH] drivers: optee: rpmb: fix returning CID to TEE

[Jorge Ramirez-Ortiz]

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

> 
>>
>> 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
>>>>
>>