[PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Andy Shevchenko]

Dereferencing something to uuid_t value is not good idea strictly speaking.
Provide a special parameter field for UUID values and drop ugly casting.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tee/optee/call.c      | 2 +-
 drivers/tee/optee/optee_msg.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 1f0a68381656..d50cff7a9406 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -241,7 +241,7 @@ int optee_open_session(struct tee_context *ctx,
 	memcpy(&msg_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
 	msg_arg->params[1].u.value.c = arg->clnt_login;
 
-	rc = tee_session_calc_client_uuid((uuid_t *)&msg_arg->params[1].u.value,
+	rc = tee_session_calc_client_uuid(&msg_arg->params[1].u.uuid,
 					  arg->clnt_login, arg->clnt_uuid);
 	if (rc)
 		goto out;
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 81ff593ac4ec..df095a974f3f 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -144,6 +144,7 @@ struct optee_msg_param_value {
  * @tmem:	parameter by temporary memory reference
  * @rmem:	parameter by registered memory reference
  * @value:	parameter by opaque value
+ * @uuid:	parameter by UUID
  *
  * @attr & OPTEE_MSG_ATTR_TYPE_MASK indicates if tmem, rmem or value is used in
  * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value,
@@ -157,6 +158,7 @@ struct optee_msg_param {
 		struct optee_msg_param_tmem tmem;
 		struct optee_msg_param_rmem rmem;
 		struct optee_msg_param_value value;
+		uuid_t uuid;
 	} u;
 };
 
-- 
2.30.2

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Jens Wiklander]

Hi Andy,

On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Dereferencing something to uuid_t value is not good idea strictly speaking.
> Provide a special parameter field for UUID values and drop ugly casting.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tee/optee/call.c      | 2 +-
>  drivers/tee/optee/optee_msg.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 1f0a68381656..d50cff7a9406 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -241,7 +241,7 @@ int optee_open_session(struct tee_context *ctx,
>         memcpy(&msg_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
>         msg_arg->params[1].u.value.c = arg->clnt_login;
>
> -       rc = tee_session_calc_client_uuid((uuid_t *)&msg_arg->params[1].u.value,
> +       rc = tee_session_calc_client_uuid(&msg_arg->params[1].u.uuid,
>                                           arg->clnt_login, arg->clnt_uuid);
>         if (rc)
>                 goto out;
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 81ff593ac4ec..df095a974f3f 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -144,6 +144,7 @@ struct optee_msg_param_value {
>   * @tmem:      parameter by temporary memory reference
>   * @rmem:      parameter by registered memory reference
>   * @value:     parameter by opaque value
> + * @uuid:      parameter by UUID
>   *
>   * @attr & OPTEE_MSG_ATTR_TYPE_MASK indicates if tmem, rmem or value is used in
>   * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value,
> @@ -157,6 +158,7 @@ struct optee_msg_param {
>                 struct optee_msg_param_tmem tmem;
>                 struct optee_msg_param_rmem rmem;
>                 struct optee_msg_param_value value;
> +               uuid_t uuid;

It's nice to get rid of the cast above, but I'm not that keen on the
change in this struct. This file defines the ABI towards Secure world
and adding dependencies on external complex types is a larger problem
than the cast above in my opinion.

Cheers,
Jens

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Andy Shevchenko]

On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

Thanks for review, my answer below.

> >                 struct optee_msg_param_tmem tmem;
> >                 struct optee_msg_param_rmem rmem;
> >                 struct optee_msg_param_value value;
> > +               uuid_t uuid;
> 
> It's nice to get rid of the cast above, but I'm not that keen on the
> change in this struct. This file defines the ABI towards Secure world
> and adding dependencies on external complex types is a larger problem
> than the cast above in my opinion.

I understand.

So, the cast is simply wrong there. Can you add a comment above that cast to
explain that and make it is marked as FIXME? Because there is no guarantee that
internal Linux types can be 1:1 mapped to the ABI of something.

What you need, perhaps, is a middle layer function that will copy u64 data
to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
variants are not in use?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Jens Wiklander]

On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
>
> Thanks for review, my answer below.
>
> > >                 struct optee_msg_param_tmem tmem;
> > >                 struct optee_msg_param_rmem rmem;
> > >                 struct optee_msg_param_value value;
> > > +               uuid_t uuid;
> >
> > It's nice to get rid of the cast above, but I'm not that keen on the
> > change in this struct. This file defines the ABI towards Secure world
> > and adding dependencies on external complex types is a larger problem
> > than the cast above in my opinion.
>
> I understand.
>
> So, the cast is simply wrong there. Can you add a comment above that cast to
> explain that and make it is marked as FIXME? Because there is no guarantee that
> internal Linux types can be 1:1 mapped to the ABI of something.

We might as well fix it directly instead. How about storing the
intermediate result in a proper uuid_t and then export it as:
export_uuid((u8 *)&msg_arg->params[1].u.uuid, &myuuid);

>
> What you need, perhaps, is a middle layer function that will copy u64 data
> to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> variants are not in use?

Does it make any difference? The file isn't shared with user space and
I need to sync the file manually anyway since OP-TEE doesn't have the
same include files.

Cheers,
Jens

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Andy Shevchenko]

On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
> On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Thanks for review, my answer below.
> >
> > > >                 struct optee_msg_param_tmem tmem;
> > > >                 struct optee_msg_param_rmem rmem;
> > > >                 struct optee_msg_param_value value;
> > > > +               uuid_t uuid;
> > >
> > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > change in this struct. This file defines the ABI towards Secure world
> > > and adding dependencies on external complex types is a larger problem
> > > than the cast above in my opinion.
> >
> > I understand.
> >
> > So, the cast is simply wrong there. Can you add a comment above that cast to
> > explain that and make it is marked as FIXME? Because there is no guarantee that
> > internal Linux types can be 1:1 mapped to the ABI of something.
>
> We might as well fix it directly instead. How about storing the
> intermediate result in a proper uuid_t and then export it as:
> export_uuid((u8 *)&msg_arg->params[1].u.uuid, &myuuid);

Still a casting here.
With u64 members you have a (potential) endianness issue (consider
BE-32 platform). Also you never know that a b c translates properly to
byte array.

I would rather see a custom function

optee_import_uuid(param, uuid_t *uuid)
{
  u8 uuid_raw[UUID_SIZE];

  put_unaligned_le64(&uuid_raw[0], param.a); // not sure about endianness
  put_unaligned_le64(&uuid_raw[0], param.b); // ditto

  import_uuid();
}

> > What you need, perhaps, is a middle layer function that will copy u64 data
> > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > variants are not in use?
>
> Does it make any difference? The file isn't shared with user space and
> I need to sync the file manually anyway since OP-TEE doesn't have the
> same include files.

Yes. It gives a hint that these are ABI (that's why I felt free to add
a member to the union. I have no idea that's an ABI). Optionally a
comment suggesting that.

Besides the above mentioned issues.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Jens Wiklander]

On Mon, Apr 19, 2021 at 3:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> > On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > Thanks for review, my answer below.
> > >
> > > > >                 struct optee_msg_param_tmem tmem;
> > > > >                 struct optee_msg_param_rmem rmem;
> > > > >                 struct optee_msg_param_value value;
> > > > > +               uuid_t uuid;
> > > >
> > > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > > change in this struct. This file defines the ABI towards Secure world
> > > > and adding dependencies on external complex types is a larger problem
> > > > than the cast above in my opinion.
> > >
> > > I understand.
> > >
> > > So, the cast is simply wrong there. Can you add a comment above that cast to
> > > explain that and make it is marked as FIXME? Because there is no guarantee that
> > > internal Linux types can be 1:1 mapped to the ABI of something.
> >
> > We might as well fix it directly instead. How about storing the
> > intermediate result in a proper uuid_t and then export it as:
> > export_uuid((u8 *)&msg_arg->params[1].u.uuid, &myuuid);
>
> Still a casting here.
> With u64 members you have a (potential) endianness issue (consider
> BE-32 platform). Also you never know that a b c translates properly to
> byte array.
>
> I would rather see a custom function
>
> optee_import_uuid(param, uuid_t *uuid)
> {
>   u8 uuid_raw[UUID_SIZE];
>
>   put_unaligned_le64(&uuid_raw[0], param.a); // not sure about endianness
>   put_unaligned_le64(&uuid_raw[0], param.b); // ditto

I believe it's a memcpy() we want then, since UUIDs are supposed to be
transmitted using a big endian memory pattern.
We should perhaps add
u8 octets[24];
to that union. Then should the result be well defined using export_uuid().

>
>   import_uuid();
> }
>
> > > What you need, perhaps, is a middle layer function that will copy u64 data
> > > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > > variants are not in use?
> >
> > Does it make any difference? The file isn't shared with user space and
> > I need to sync the file manually anyway since OP-TEE doesn't have the
> > same include files.
>
> Yes. It gives a hint that these are ABI (that's why I felt free to add
> a member to the union. I have no idea that's an ABI). Optionally a
> comment suggesting that.

It does say that it defines a protocol at the beginning of the file, I
can add ABI too if you think that helps.

Cheers,
Jens

Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values

[Andy Shevchenko]

On Mon, Apr 19, 2021 at 5:30 PM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
> On Mon, Apr 19, 2021 at 3:40 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
> > <jens.wiklander@linaro.org> wrote:
> > > On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > Thanks for review, my answer below.
> > > >
> > > > > >                 struct optee_msg_param_tmem tmem;
> > > > > >                 struct optee_msg_param_rmem rmem;
> > > > > >                 struct optee_msg_param_value value;
> > > > > > +               uuid_t uuid;
> > > > >
> > > > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > > > change in this struct. This file defines the ABI towards Secure world
> > > > > and adding dependencies on external complex types is a larger problem
> > > > > than the cast above in my opinion.
> > > >
> > > > I understand.
> > > >
> > > > So, the cast is simply wrong there. Can you add a comment above that cast to
> > > > explain that and make it is marked as FIXME? Because there is no guarantee that
> > > > internal Linux types can be 1:1 mapped to the ABI of something.
> > >
> > > We might as well fix it directly instead. How about storing the
> > > intermediate result in a proper uuid_t and then export it as:
> > > export_uuid((u8 *)&msg_arg->params[1].u.uuid, &myuuid);
> >
> > Still a casting here.
> > With u64 members you have a (potential) endianness issue (consider
> > BE-32 platform). Also you never know that a b c translates properly to
> > byte array.
> >
> > I would rather see a custom function
> >
> > optee_import_uuid(param, uuid_t *uuid)
> > {
> >   u8 uuid_raw[UUID_SIZE];
> >
> >   put_unaligned_le64(&uuid_raw[0], param.a); // not sure about endianness
> >   put_unaligned_le64(&uuid_raw[0], param.b); // ditto
>
> I believe it's a memcpy() we want then, since UUIDs are supposed to be
> transmitted using a big endian memory pattern.
> We should perhaps add
> u8 octets[24];
> to that union. Then should the result be well defined using export_uuid().

Right, if you do that, it would be wonderful!

> >   import_uuid();
> > }
> >
> > > > What you need, perhaps, is a middle layer function that will copy u64 data
> > > > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > > > variants are not in use?
> > >
> > > Does it make any difference? The file isn't shared with user space and
> > > I need to sync the file manually anyway since OP-TEE doesn't have the
> > > same include files.
> >
> > Yes. It gives a hint that these are ABI (that's why I felt free to add
> > a member to the union. I have no idea that's an ABI). Optionally a
> > comment suggesting that.
>
> It does say that it defines a protocol at the beginning of the file, I
> can add ABI too if you think that helps.

I read the structure definition, perhaps some clarification on a data
type level would be nice.

Thanks!

-- 
With Best Regards,
Andy Shevchenko