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; };
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
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?
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
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.
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
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!
op-tee@lists.trustedfirmware.org
-
Andy Shevchenko -
Andy Shevchenko -
Jens Wiklander