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

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

[Jorge Ramirez-Ortiz]

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]);
+
 	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

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

[Jens Wiklander]

Hi Jorge,

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

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
>

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

[Jorge Ramirez-Ortiz]

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.


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

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?


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

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

[Jens Wiklander]

[+ 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.

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

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

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

[Jens Wiklander]

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

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.

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

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

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

[Jorge Ramirez-Ortiz]

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@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
> 
> 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(&regs->cmdrsp3);
		cmdrsp2 = esdhc_read32(&regs->cmdrsp2);
		cmdrsp1 = esdhc_read32(&regs->cmdrsp1);
		cmdrsp0 = esdhc_read32(&regs->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
>>>>>>
>>>>
>>

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

[Jorge Ramirez-Ortiz]

On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> 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@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
>>
>> 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.


Yeah, actually it is: but perhaps should be fixed in the Linux
supplicant instead.

Both u-boot and Linux do read the CID properly from MMC and they both
hold the same values in four u32 variables so I can confirm that the MMC
drivers for the imx do the right thing

However in the trusted environment the situation is a bit different:

1) when Linux reports it to sysfs, Linux displays the CID as _four_
concatenated u32 values (not as an array of sixteen u8 values).

2) The Linux TEE supplicant reads said entry as an array of u8 therefore
discarding the endianess.

3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
therefore keeping the endiannes.

It is clear that at this point, the value that will reach the OPTEE's
rpmb driver from linux will be different to the one from uboot.

So we could either fix it in u-boot's RPMB driver (with the patch I
posted) or in the Linux supplicant in the  read_cid(..) function.

But one of the two has to change  not only for consistency but to enable
both u-boot and Linux to access rpmb during the boot process on any
endian systems.

what do you think? does this make sense?

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

[Jens Wiklander]

On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> > 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@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
> >>
> >> 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.
> 
> 
> Yeah, actually it is: but perhaps should be fixed in the Linux
> supplicant instead.
> 
> Both u-boot and Linux do read the CID properly from MMC and they both
> hold the same values in four u32 variables so I can confirm that the MMC
> drivers for the imx do the right thing

Good

> 
> However in the trusted environment the situation is a bit different:
> 
> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> concatenated u32 values (not as an array of sixteen u8 values).
> 
> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> discarding the endianess.
> 
> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> therefore keeping the endiannes.
> 
> It is clear that at this point, the value that will reach the OPTEE's
> rpmb driver from linux will be different to the one from uboot.
> 
> So we could either fix it in u-boot's RPMB driver (with the patch I
> posted) or in the Linux supplicant in the  read_cid(..) function.
> 
> But one of the two has to change  not only for consistency but to enable
> both u-boot and Linux to access rpmb during the boot process on any
> endian systems.
> 
> what do you think? does this make sense?
> 

Thanks for digging into this, now the problem is clear to me. At the
Linux side I think the CID is received by secure world with the bytes in
the expected order. You're original patch fixes this by byte swapping
the words as needed. However, I think that cpu_to_be32() should be used
instead for clarity. Then there's the issue of alignment with the
casting you do. It works today due to how the function is called, but
the compiler can't guarantee that since the struct rpmb_dev_info only
contains u8:s so it's only byte aligned. You need to handle that inside
the function.

Cheers,
Jens

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

[Jorge Ramirez-Ortiz]

On 11/20/19 8:20 AM, Jens Wiklander wrote:
> On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
>> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
>>> 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@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
>>>>
>>>> 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.
>>
>>
>> Yeah, actually it is: but perhaps should be fixed in the Linux
>> supplicant instead.
>>
>> Both u-boot and Linux do read the CID properly from MMC and they both
>> hold the same values in four u32 variables so I can confirm that the MMC
>> drivers for the imx do the right thing
> 
> Good
> 
>>
>> However in the trusted environment the situation is a bit different:
>>
>> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
>> concatenated u32 values (not as an array of sixteen u8 values).
>>
>> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
>> discarding the endianess.
>>
>> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
>> therefore keeping the endiannes.
>>
>> It is clear that at this point, the value that will reach the OPTEE's
>> rpmb driver from linux will be different to the one from uboot.
>>
>> So we could either fix it in u-boot's RPMB driver (with the patch I
>> posted) or in the Linux supplicant in the  read_cid(..) function.
>>
>> But one of the two has to change  not only for consistency but to enable
>> both u-boot and Linux to access rpmb during the boot process on any
>> endian systems.
>>
>> what do you think? does this make sense?
>>
> 
> Thanks for digging into this, now the problem is clear to me. At the
> Linux side I think the CID is received by secure world with the bytes in
> the expected order. You're original patch fixes this by byte swapping
> the words as needed. 

which incidentally is exactly the same thing that linux does when the
MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.

static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
{
	int ret, i;
	__be32 *cid_tmp;

	cid_tmp = kzalloc(16, GFP_KERNEL);
	if (!cid_tmp)
		return -ENOMEM;

	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
	if (ret)
		goto err;

	for (i = 0; i < 4; i++)
		cid[i] = be32_to_cpu(cid_tmp[i]);

err:
	kfree(cid_tmp);
	return ret;
}

However, I think that cpu_to_be32() should be used
> instead for clarity. 

sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?

Then there's the issue of alignment with the
> casting you do. It works today due to how the function is called, but
> the compiler can't guarantee that since the struct rpmb_dev_info only
> contains u8:s so it's only byte aligned. You need to handle that inside
> the function.

but why does the optee supplicant in uboot igonres the alignment request
made by optee (cmd_shm_alloc in uboot sets alignment to 0 while
thread_rpc_alloc_payload in optee requested 8)

also any reason why we cant ask some other alignment from optee when
allocating the response buffer for the mmc dev info? then we dont have
to jump through hoops in uboot?

> 
> Cheers,
> Jens
> 

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

[Jens Wiklander]

On Wed, Nov 20, 2019 at 09:21:35AM +0100, Jorge Ramirez-Ortiz wrote:
> On 11/20/19 8:20 AM, Jens Wiklander wrote:
> > On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> >> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> >>> 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@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
> >>>>
> >>>> 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.
> >>
> >>
> >> Yeah, actually it is: but perhaps should be fixed in the Linux
> >> supplicant instead.
> >>
> >> Both u-boot and Linux do read the CID properly from MMC and they both
> >> hold the same values in four u32 variables so I can confirm that the MMC
> >> drivers for the imx do the right thing
> > 
> > Good
> > 
> >>
> >> However in the trusted environment the situation is a bit different:
> >>
> >> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> >> concatenated u32 values (not as an array of sixteen u8 values).
> >>
> >> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> >> discarding the endianess.
> >>
> >> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> >> therefore keeping the endiannes.
> >>
> >> It is clear that at this point, the value that will reach the OPTEE's
> >> rpmb driver from linux will be different to the one from uboot.
> >>
> >> So we could either fix it in u-boot's RPMB driver (with the patch I
> >> posted) or in the Linux supplicant in the  read_cid(..) function.
> >>
> >> But one of the two has to change  not only for consistency but to enable
> >> both u-boot and Linux to access rpmb during the boot process on any
> >> endian systems.
> >>
> >> what do you think? does this make sense?
> >>
> > 
> > Thanks for digging into this, now the problem is clear to me. At the
> > Linux side I think the CID is received by secure world with the bytes in
> > the expected order. You're original patch fixes this by byte swapping
> > the words as needed. 
> 
> which incidentally is exactly the same thing that linux does when the
> MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.
> 
> static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
> {
> 	int ret, i;
> 	__be32 *cid_tmp;
> 
> 	cid_tmp = kzalloc(16, GFP_KERNEL);
> 	if (!cid_tmp)
> 		return -ENOMEM;
> 
> 	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
> 	if (ret)
> 		goto err;
> 
> 	for (i = 0; i < 4; i++)
> 		cid[i] = be32_to_cpu(cid_tmp[i]);
> 
> err:
> 	kfree(cid_tmp);
> 	return ret;
> }
> 
> However, I think that cpu_to_be32() should be used
> > instead for clarity. 
> 
> sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?

Yes, the words are in little endian but we need them to be in big endian
when making it an array of u8.

> 
> Then there's the issue of alignment with the
> > casting you do. It works today due to how the function is called, but
> > the compiler can't guarantee that since the struct rpmb_dev_info only
> > contains u8:s so it's only byte aligned. You need to handle that inside
> > the function.
> 
> but why does the optee supplicant in uboot igonres the alignment request
> made by optee (cmd_shm_alloc in uboot sets alignment to 0 while
> thread_rpc_alloc_payload in optee requested 8)

Good question, looks like a bug to me. In this case it doesn't make much
difference though since malloc() is required to return buffers which are
at least 8 byte aligned.

> 
> also any reason why we cant ask some other alignment from optee when
> allocating the response buffer for the mmc dev info? then we dont have
> to jump through hoops in uboot?

You mean the cache line aligned buffers? I don't see a problem with that,
a patch is welcome. :-)

Any way, the suspicious cast which you're doing will cause a compiler
warning with the right flags enabled. If you'd rather write a long
comment explaining why it's perfectly safe to cast like that do so
instead.

Cheers,
Jens

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

[Jorge]

On 20/11/19 11:33:10, Jens Wiklander wrote:
> On Wed, Nov 20, 2019 at 09:21:35AM +0100, Jorge Ramirez-Ortiz wrote:
> > On 11/20/19 8:20 AM, Jens Wiklander wrote:
> > > On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> > >> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> > >>> 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@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
> > >>>>
> > >>>> 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.
> > >>
> > >>
> > >> Yeah, actually it is: but perhaps should be fixed in the Linux
> > >> supplicant instead.
> > >>
> > >> Both u-boot and Linux do read the CID properly from MMC and they both
> > >> hold the same values in four u32 variables so I can confirm that the MMC
> > >> drivers for the imx do the right thing
> > > 
> > > Good
> > > 
> > >>
> > >> However in the trusted environment the situation is a bit different:
> > >>
> > >> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> > >> concatenated u32 values (not as an array of sixteen u8 values).
> > >>
> > >> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> > >> discarding the endianess.
> > >>
> > >> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> > >> therefore keeping the endiannes.
> > >>
> > >> It is clear that at this point, the value that will reach the OPTEE's
> > >> rpmb driver from linux will be different to the one from uboot.
> > >>
> > >> So we could either fix it in u-boot's RPMB driver (with the patch I
> > >> posted) or in the Linux supplicant in the  read_cid(..) function.
> > >>
> > >> But one of the two has to change  not only for consistency but to enable
> > >> both u-boot and Linux to access rpmb during the boot process on any
> > >> endian systems.
> > >>
> > >> what do you think? does this make sense?
> > >>
> > > 
> > > Thanks for digging into this, now the problem is clear to me. At the
> > > Linux side I think the CID is received by secure world with the bytes in
> > > the expected order. You're original patch fixes this by byte swapping
> > > the words as needed.

right, because the supplicant is implicitly doing the swap by picking
one byte at a time since the linux kernel wrote u32s and not bytes to
sysfs.so between them things balanced out.


> > 
> > which incidentally is exactly the same thing that linux does when the
> > MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.
> > 
> > static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
> > {
> > 	int ret, i;
> > 	__be32 *cid_tmp;
> > 
> > 	cid_tmp = kzalloc(16, GFP_KERNEL);
> > 	if (!cid_tmp)
> > 		return -ENOMEM;
> > 
> > 	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
> > 	if (ret)
> > 		goto err;
> > 
> > 	for (i = 0; i < 4; i++)
> > 		cid[i] = be32_to_cpu(cid_tmp[i]);
> > 
> > err:
> > 	kfree(cid_tmp);
> > 	return ret;
> > }
> > 
> > However, I think that cpu_to_be32() should be used
> > > instead for clarity. 
> > 
> > sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?
> 
> Yes, the words are in little endian but we need them to be in big endian
> when making it an array of u8.

no, sorry, I dont understand this. it would not work: we have to have
be32_to_cpu

really it is either the linux supplicant or uboot that have to
change. I supose uboot will be the preferred choice since it will have
less impact on current users.

> 
> > 
> > Then there's the issue of alignment with the
> > > casting you do. It works today due to how the function is called, but
> > > the compiler can't guarantee that since the struct rpmb_dev_info only
> > > contains u8:s so it's only byte aligned. You need to handle that inside
> > > the function.
> > 
> > but why does the optee supplicant in uboot igonres the alignment request
> > made by optee (cmd_shm_alloc in uboot sets alignment to 0 while
> > thread_rpc_alloc_payload in optee requested 8)
> 
> Good question, looks like a bug to me. In this case it doesn't make much
> difference though since malloc() is required to return buffers which are
> at least 8 byte aligned.
> 
> > 
> > also any reason why we cant ask some other alignment from optee when
> > allocating the response buffer for the mmc dev info? then we dont have
> > to jump through hoops in uboot?
> 
> You mean the cache line aligned buffers? I don't see a problem with that,
> a patch is welcome. :-)
> 
> Any way, the suspicious cast which you're doing will cause a compiler
> warning with the right flags enabled. If you'd rather write a long
> comment explaining why it's perfectly safe to cast like that do so
> instead.
> 
> Cheers,
> Jens

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

[Jens Wiklander]

On Tue, Nov 26, 2019 at 09:22:38AM +0100, Jorge Ramirez-Ortiz, Foundries wrote:
> On 20/11/19 11:33:10, Jens Wiklander wrote:
> > On Wed, Nov 20, 2019 at 09:21:35AM +0100, Jorge Ramirez-Ortiz wrote:
> > > On 11/20/19 8:20 AM, Jens Wiklander wrote:
> > > > On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> > > >> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> > > >>> 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@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
> > > >>>>
> > > >>>> 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.
> > > >>
> > > >>
> > > >> Yeah, actually it is: but perhaps should be fixed in the Linux
> > > >> supplicant instead.
> > > >>
> > > >> Both u-boot and Linux do read the CID properly from MMC and they both
> > > >> hold the same values in four u32 variables so I can confirm that the MMC
> > > >> drivers for the imx do the right thing
> > > > 
> > > > Good
> > > > 
> > > >>
> > > >> However in the trusted environment the situation is a bit different:
> > > >>
> > > >> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> > > >> concatenated u32 values (not as an array of sixteen u8 values).
> > > >>
> > > >> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> > > >> discarding the endianess.
> > > >>
> > > >> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> > > >> therefore keeping the endiannes.
> > > >>
> > > >> It is clear that at this point, the value that will reach the OPTEE's
> > > >> rpmb driver from linux will be different to the one from uboot.
> > > >>
> > > >> So we could either fix it in u-boot's RPMB driver (with the patch I
> > > >> posted) or in the Linux supplicant in the  read_cid(..) function.
> > > >>
> > > >> But one of the two has to change  not only for consistency but to enable
> > > >> both u-boot and Linux to access rpmb during the boot process on any
> > > >> endian systems.
> > > >>
> > > >> what do you think? does this make sense?
> > > >>
> > > > 
> > > > Thanks for digging into this, now the problem is clear to me. At the
> > > > Linux side I think the CID is received by secure world with the bytes in
> > > > the expected order. You're original patch fixes this by byte swapping
> > > > the words as needed.
> 
> right, because the supplicant is implicitly doing the swap by picking
> one byte at a time since the linux kernel wrote u32s and not bytes to
> sysfs.so between them things balanced out.
> 
> 
> > > 
> > > which incidentally is exactly the same thing that linux does when the
> > > MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.
> > > 
> > > static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
> > > {
> > > 	int ret, i;
> > > 	__be32 *cid_tmp;
> > > 
> > > 	cid_tmp = kzalloc(16, GFP_KERNEL);
> > > 	if (!cid_tmp)
> > > 		return -ENOMEM;
> > > 
> > > 	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
> > > 	if (ret)
> > > 		goto err;
> > > 
> > > 	for (i = 0; i < 4; i++)
> > > 		cid[i] = be32_to_cpu(cid_tmp[i]);
> > > 
> > > err:
> > > 	kfree(cid_tmp);
> > > 	return ret;
> > > }
> > > 
> > > However, I think that cpu_to_be32() should be used
> > > > instead for clarity. 
> > > 
> > > sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?
> > 
> > Yes, the words are in little endian but we need them to be in big endian
> > when making it an array of u8.
> 
> no, sorry, I dont understand this. it would not work: we have to have
> be32_to_cpu

On a little endian system cpu_to_be32() and be32_to_cpu() are both
implemented using uswap_32(). The only difference is in usage. With
cpu_to_be32() you have something in native byte order and convert it to
big endian, with be32_to_cpu() it's the other way around.

In this case (in rpmb_get_dev_info() far up in this conversion) we have
the 4 words in native endian and we need to convert them into big
endian.

Cheers,
Jens

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

[Jorge]

On 26/11/19 12:46:04, Jens Wiklander wrote:
> On Tue, Nov 26, 2019 at 09:22:38AM +0100, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 20/11/19 11:33:10, Jens Wiklander wrote:
> > > On Wed, Nov 20, 2019 at 09:21:35AM +0100, Jorge Ramirez-Ortiz wrote:
> > > > On 11/20/19 8:20 AM, Jens Wiklander wrote:
> > > > > On Tue, Nov 19, 2019 at 06:21:34PM +0100, Jorge Ramirez-Ortiz wrote:
> > > > >> On 11/19/19 12:53 PM, Jorge Ramirez-Ortiz wrote:
> > > > >>> 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@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
> > > > >>>>
> > > > >>>> 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.
> > > > >>
> > > > >>
> > > > >> Yeah, actually it is: but perhaps should be fixed in the Linux
> > > > >> supplicant instead.
> > > > >>
> > > > >> Both u-boot and Linux do read the CID properly from MMC and they both
> > > > >> hold the same values in four u32 variables so I can confirm that the MMC
> > > > >> drivers for the imx do the right thing
> > > > > 
> > > > > Good
> > > > > 
> > > > >>
> > > > >> However in the trusted environment the situation is a bit different:
> > > > >>
> > > > >> 1) when Linux reports it to sysfs, Linux displays the CID as _four_
> > > > >> concatenated u32 values (not as an array of sixteen u8 values).
> > > > >>
> > > > >> 2) The Linux TEE supplicant reads said entry as an array of u8 therefore
> > > > >> discarding the endianess.
> > > > >>
> > > > >> 3) In U-boot the rpmb.c driver does memcpy the cid uint32 array into u8
> > > > >> therefore keeping the endiannes.
> > > > >>
> > > > >> It is clear that at this point, the value that will reach the OPTEE's
> > > > >> rpmb driver from linux will be different to the one from uboot.
> > > > >>
> > > > >> So we could either fix it in u-boot's RPMB driver (with the patch I
> > > > >> posted) or in the Linux supplicant in the  read_cid(..) function.
> > > > >>
> > > > >> But one of the two has to change  not only for consistency but to enable
> > > > >> both u-boot and Linux to access rpmb during the boot process on any
> > > > >> endian systems.
> > > > >>
> > > > >> what do you think? does this make sense?
> > > > >>
> > > > > 
> > > > > Thanks for digging into this, now the problem is clear to me. At the
> > > > > Linux side I think the CID is received by secure world with the bytes in
> > > > > the expected order. You're original patch fixes this by byte swapping
> > > > > the words as needed.
> > 
> > right, because the supplicant is implicitly doing the swap by picking
> > one byte at a time since the linux kernel wrote u32s and not bytes to
> > sysfs.so between them things balanced out.
> > 
> > 
> > > > 
> > > > which incidentally is exactly the same thing that linux does when the
> > > > MMC host talks the SPI protocol ie, be32_to_cpu on each of the 4 cid words.
> > > > 
> > > > static int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
> > > > {
> > > > 	int ret, i;
> > > > 	__be32 *cid_tmp;
> > > > 
> > > > 	cid_tmp = kzalloc(16, GFP_KERNEL);
> > > > 	if (!cid_tmp)
> > > > 		return -ENOMEM;
> > > > 
> > > > 	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
> > > > 	if (ret)
> > > > 		goto err;
> > > > 
> > > > 	for (i = 0; i < 4; i++)
> > > > 		cid[i] = be32_to_cpu(cid_tmp[i]);
> > > > 
> > > > err:
> > > > 	kfree(cid_tmp);
> > > > 	return ret;
> > > > }
> > > > 
> > > > However, I think that cpu_to_be32() should be used
> > > > > instead for clarity. 
> > > > 
> > > > sorry what do you mean? cpu_to_be32 instead of be32_to_cpu?
> > > 
> > > Yes, the words are in little endian but we need them to be in big endian
> > > when making it an array of u8.
> > 
> > no, sorry, I dont understand this. it would not work: we have to have
> > be32_to_cpu
> 
> On a little endian system cpu_to_be32() and be32_to_cpu() are both
> implemented using uswap_32(). The only difference is in usage. With
> cpu_to_be32() you have something in native byte order and convert it to
> big endian, with be32_to_cpu() it's the other way around.
> 
> In this case (in rpmb_get_dev_info() far up in this conversion) we have
> the 4 words in native endian and we need to convert them into big
> endian.

yes, of course you are right (I double checked those implementations
in LE and I am with you now).

will repost the patch

thanks!

> 
> Cheers,
> Jens

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

[Jorge Ramirez-Ortiz]

The mmc CID value is one of the input parameters used to provision the
RPMB key. The trusted execution environment expects this value to be
specified in big endian format.

Before this fix, on little endian systems, the value returned by the
linux kernel mmc driver differed from the one returned by u-boot.
This meant that if linux provisioned the RPMB key, u-boot would not
have access to the partition (and the other way around).

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..cf1ce77e6e 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] = cpu_to_be32(mmc->cid[i]);
+
 	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.17.1

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

[Jens Wiklander]

On Tue, Nov 26, 2019 at 05:19:34PM +0100, Jorge Ramirez-Ortiz wrote:
> The mmc CID value is one of the input parameters used to provision the
> RPMB key. The trusted execution environment expects this value to be
> specified in big endian format.
> 
> Before this fix, on little endian systems, the value returned by the
> linux kernel mmc driver differed from the one returned by u-boot.
> This meant that if linux provisioned the RPMB key, u-boot would not
> have access to the partition (and the other way around).
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

Looks good to me:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks,
Jens

> ---
>  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..cf1ce77e6e 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] = cpu_to_be32(mmc->cid[i]);
> +
>  	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.17.1
> 

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

[Tom Rini]

On Tue, Nov 26, 2019 at 05:19:34PM +0100, Jorge Ramirez-Ortiz wrote:

> The mmc CID value is one of the input parameters used to provision the
> RPMB key. The trusted execution environment expects this value to be
> specified in big endian format.
> 
> Before this fix, on little endian systems, the value returned by the
> linux kernel mmc driver differed from the one returned by u-boot.
> This meant that if linux provisioned the RPMB key, u-boot would not
> have access to the partition (and the other way around).
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191205/46b46695/attachment.sig>