Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on the kernel mailing lists. The last is from Mark Rutland: https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something) criticizing the interface, but everyone has more or less asked us to reconsider.
I think it's time to take one step back and reevaluate the consequences of having a well defined user space API.
Consider dropping TEE_IOC_CMD and replace it with: struct tee_open_session_arg { __u8 uuid[UUID_LEN]; __u8 clnt_uuid[UUID_LEN]; __u32 clnt_login; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg { __u32 func; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg { __u32 cookie; /* matches the command or open session to cancel */ __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg { __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CLOSE_SESSION
The tee_open_session_arg and tee_invoke_command_arg structs would be followed by a struct msg_param as defined in optee_msg.h in patch v3 https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to: #define TEE_IMPL_ID_OPTEE 1 #define TEE_OPTEE_CAP_TZ (1 << 0) struct tee_ioctl_version_data { __u32 impl_id; __u32 capabilities; }; or equivalent.
Advantages: 1. User space gets a well defined interface 2. It will be easier to create a uniform API for the coming kernel internal API 3. The subsystem could offload more from the driver.
Disadvantages: 1. Risk that we'll need to extend the API if a field is missing for some TEE.
I'd like some feedback on providing a well defined user space API. Note that the user space API shouldn't enforce a certain protocol between normal and secure world.
-- Thanks, Jens
Hi,
On 08 Jun 2015, at 15:05, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on the kernel mailing lists. The last is from Mark Rutland: https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something) criticizing the interface, but everyone has more or less asked us to reconsider.
I think it's time to take one step back and reevaluate the consequences of having a well defined user space API.
I believe that this is in a way the same problem that we discussed regarding an in-kernel API. Then it would be immediate to map to a user-space API (plus, we have a strong argument to put the driver in the kernel in the first place).
Consider dropping TEE_IOC_CMD and replace it with: struct tee_open_session_arg { __u8 uuid[UUID_LEN]; __u8 clnt_uuid[UUID_LEN]; __u32 clnt_login; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg { __u32 func; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg { __u32 cookie; /* matches the command or open session to cancel */ __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg { __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CLOSE_SESSION
I would not use TEE naming (i.e., _session); open/close/send(invoke) should be enough. Also, we have seen that shared memory operations are common to the TEEs we have worked with, so I would suggest to add them to the interface too.
The tee_open_session_arg and tee_invoke_command_arg structs would be followed by a struct msg_param as defined in optee_msg.h in patch v3 https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to: #define TEE_IMPL_ID_OPTEE 1 #define TEE_OPTEE_CAP_TZ (1 << 0) struct tee_ioctl_version_data { __u32 impl_id; __u32 capabilities; }; or equivalent.
Advantages:
- User space gets a well defined interface
- It will be easier to create a uniform API for the coming kernel internal
API 3. The subsystem could offload more from the driver.
+1
Disadvantages:
- Risk that we'll need to extend the API if a field is missing for some
TEE.
We should be able to define read/write/send pretty good to start with - they are not that complicated. Then we can add more functionality to the interface as we define new trusted services that we want to expose to kernel submodules and user space.
I'd like some feedback on providing a well defined user space API. Note that the user space API shouldn't enforce a certain protocol between normal and secure world.
As I mentioned I think that treating user and kernel space separately is an error, since we risk to end up with a fragmented interface (this is indeed one of the comments we got regarding kernel subsystems). I would send a revision of the driver where the kernel interface is explicit and ioctls map to them (after converting to kernel memory space, etc.).
-- Thanks, Jens _______________________________________________ Tee-dev mailing list Tee-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/tee-dev
Best, Javier.
Javier
On Mon, Jun 08, 2015 at 03:23:52PM +0200, Javier González wrote:
Hi,
On 08 Jun 2015, at 15:05, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on the kernel mailing lists. The last is from Mark Rutland: https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something) criticizing the interface, but everyone has more or less asked us to reconsider.
I think it's time to take one step back and reevaluate the consequences of having a well defined user space API.
I believe that this is in a way the same problem that we discussed regarding an in-kernel API. Then it would be immediate to map to a user-space API (plus, we have a strong argument to put the driver in the kernel in the first place).
Consider dropping TEE_IOC_CMD and replace it with: struct tee_open_session_arg { __u8 uuid[UUID_LEN]; __u8 clnt_uuid[UUID_LEN]; __u32 clnt_login; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg { __u32 func; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg { __u32 cookie; /* matches the command or open session to cancel */ __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg { __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CLOSE_SESSION
I would not use TEE naming (i.e., _session); open/close/send(invoke) should be enough.
If we drop _session, wouldn't that cause some confusion? Consider the following sequence:
fd = open("/dev/tee0"); ioctl(fd, TEE_IOC_OPEN, &arg);
What would be the intuitive interpretation of TEE_IOC_OPEN in this sequence? I would prefer keeping the _session suffix here to be clear that it's a session we're starting and ending. It doesn't have to be "_session", anything which makes it obvious that it has nothing to do with open(2) and close(2) is fine.
For invoke and cancel it makes sense to shorten them to TEE_IOC_INVOKE and TEE_IOC_CANCEL. We should avoid send as it implies that it could be asynchronous, which it isn't.
We'll have to add two IOCs for tee-supplicant also: struct tee_suppl_arg { union { __u32 func; __u32 ret; } u; __u32 num_params; }; TEE_IOC_SUPPL_RECV TEE_IOC_SUPPL_SEND
Both followed by a struct msg_param in the same way as for open_session and invoke above. u.func is used by RECV and u.ret is used by SEND
If supplicant serves with several threads it should open the device once for each thread. One particular TEE_IOC_SUPPL_RECV on a file descriptor should always be followed by one TEE_IOC_SUPPL_SEND. The driver may refuse opening the device several times if it doesn't support multi-threading.
Also, we have seen that shared memory operations are common to the TEEs we have worked with, so I would suggest to add them to the interface too.
Those are already there, in include/uapi/linux/tee.h
The tee_open_session_arg and tee_invoke_command_arg structs would be followed by a struct msg_param as defined in optee_msg.h in patch v3 https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to: #define TEE_IMPL_ID_OPTEE 1 #define TEE_OPTEE_CAP_TZ (1 << 0) struct tee_ioctl_version_data { __u32 impl_id; __u32 capabilities; }; or equivalent.
Advantages:
- User space gets a well defined interface
- It will be easier to create a uniform API for the coming kernel internal
API 3. The subsystem could offload more from the driver.
+1
Disadvantages:
- Risk that we'll need to extend the API if a field is missing for some
TEE.
We should be able to define read/write/send pretty good to start with - they are not that complicated. Then we can add more functionality to the interface as we define new trusted services that we want to expose to kernel submodules and user space.
+1
I'd like some feedback on providing a well defined user space API. Note that the user space API shouldn't enforce a certain protocol between normal and secure world.
As I mentioned I think that treating user and kernel space separately is an error, since we risk to end up with a fragmented interface (this is indeed one of the comments we got regarding kernel subsystems). I would send a revision of the driver where the kernel interface is explicit and ioctls map to them (after converting to kernel memory space, etc.).
Agree, I'm thinking that the specific driver will provide functions in struct tee_driver_ops that matches the TEE_IOC_{OPEN_SESSION,CLOSE_SESSION,INVOKE,CANCEL}. Those functions could then be reused in some way for the kernel interface too.
-- Thanks, Jens
Hi,
On 09 Jun 2015, at 12:41, Jens Wiklander jens.wiklander@linaro.org wrote:
On Mon, Jun 08, 2015 at 03:23:52PM +0200, Javier González wrote:
Hi,
On 08 Jun 2015, at 15:05, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on the kernel mailing lists. The last is from Mark Rutland: https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something) criticizing the interface, but everyone has more or less asked us to reconsider.
I think it's time to take one step back and reevaluate the consequences of having a well defined user space API.
I believe that this is in a way the same problem that we discussed regarding an in-kernel API. Then it would be immediate to map to a user-space API (plus, we have a strong argument to put the driver in the kernel in the first place).
Consider dropping TEE_IOC_CMD and replace it with: struct tee_open_session_arg { __u8 uuid[UUID_LEN]; __u8 clnt_uuid[UUID_LEN]; __u32 clnt_login; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg { __u32 func; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg { __u32 cookie; /* matches the command or open session to cancel */ __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg { __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CLOSE_SESSION
I would not use TEE naming (i.e., _session); open/close/send(invoke) should be enough.
If we drop _session, wouldn't that cause some confusion? Consider the following sequence:
fd = open("/dev/tee0"); ioctl(fd, TEE_IOC_OPEN, &arg);
What would be the intuitive interpretation of TEE_IOC_OPEN in this sequence? I would prefer keeping the _session suffix here to be clear that it's a session we're starting and ending. It doesn't have to be "_session", anything which makes it obvious that it has nothing to do with open(2) and close(2) is fine.
Ok. I am generally reluctant to maintain GP’s terminology, but I see your point.
For invoke and cancel it makes sense to shorten them to TEE_IOC_INVOKE and TEE_IOC_CANCEL. We should avoid send as it implies that it could be asynchronous, which it isn’t.
You are right, invoke is a better name for sync. communications. I have seen asynchronous TA’s in other contexts, but it is not relevant as long as we only support OP-TEE and Open Virtualization.
We'll have to add two IOCs for tee-supplicant also: struct tee_suppl_arg { union { __u32 func; __u32 ret; } u; __u32 num_params; }; TEE_IOC_SUPPL_RECV TEE_IOC_SUPPL_SEND
Both followed by a struct msg_param in the same way as for open_session and invoke above. u.func is used by RECV and u.ret is used by SEND
+1
If supplicant serves with several threads it should open the device once for each thread. One particular TEE_IOC_SUPPL_RECV on a file descriptor should always be followed by one TEE_IOC_SUPPL_SEND. The driver may refuse opening the device several times if it doesn't support multi-threading.
+1 This should be the same for kernel space.
Also, we have seen that shared memory operations are common to the TEEs we have worked with, so I would suggest to add them to the interface too.
Those are already there, in include/uapi/linux/tee.h
Yes. What I meant is that they should stay there; it is “add” to the commands you mentioned above (tee_cancel_command_arg, tee_invoke_command_arg , etc.)
The tee_open_session_arg and tee_invoke_command_arg structs would be followed by a struct msg_param as defined in optee_msg.h in patch v3 https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to: #define TEE_IMPL_ID_OPTEE 1 #define TEE_OPTEE_CAP_TZ (1 << 0) struct tee_ioctl_version_data { __u32 impl_id; __u32 capabilities; }; or equivalent.
Advantages:
- User space gets a well defined interface
- It will be easier to create a uniform API for the coming kernel internal
API 3. The subsystem could offload more from the driver.
+1
Disadvantages:
- Risk that we'll need to extend the API if a field is missing for some
TEE.
We should be able to define read/write/send pretty good to start with - they are not that complicated. Then we can add more functionality to the interface as we define new trusted services that we want to expose to kernel submodules and user space.
+1
I'd like some feedback on providing a well defined user space API. Note that the user space API shouldn't enforce a certain protocol between normal and secure world.
As I mentioned I think that treating user and kernel space separately is an error, since we risk to end up with a fragmented interface (this is indeed one of the comments we got regarding kernel subsystems). I would send a revision of the driver where the kernel interface is explicit and ioctls map to them (after converting to kernel memory space, etc.).
Agree, I'm thinking that the specific driver will provide functions in struct tee_driver_ops that matches the TEE_IOC_{OPEN_SESSION,CLOSE_SESSION,INVOKE,CANCEL}. Those functions could then be reused in some way for the kernel interface too.
Exactly.
-- Thanks, Jens
Best, Javier.
Hi,
On Tue, Jun 09, 2015 at 07:19:18PM +0200, Javier González wrote:
Hi,
On 09 Jun 2015, at 12:41, Jens Wiklander jens.wiklander@linaro.org wrote:
On Mon, Jun 08, 2015 at 03:23:52PM +0200, Javier González wrote:
Hi,
On 08 Jun 2015, at 15:05, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on the kernel mailing lists. The last is from Mark Rutland: https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something) criticizing the interface, but everyone has more or less asked us to reconsider.
I think it's time to take one step back and reevaluate the consequences of having a well defined user space API.
I believe that this is in a way the same problem that we discussed regarding an in-kernel API. Then it would be immediate to map to a user-space API (plus, we have a strong argument to put the driver in the kernel in the first place).
Consider dropping TEE_IOC_CMD and replace it with: struct tee_open_session_arg { __u8 uuid[UUID_LEN]; __u8 clnt_uuid[UUID_LEN]; __u32 clnt_login; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg { __u32 func; __u32 cookie; /* some unique value set by user space */ __u32 session; __u32 ret; __u32 ret_origin; __u32 num_params; }; TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg { __u32 cookie; /* matches the command or open session to cancel */ __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg { __u32 session; __u32 ret; __u32 ret_origin; }; TEE_IOC_CLOSE_SESSION
I would not use TEE naming (i.e., _session); open/close/send(invoke) should be enough.
If we drop _session, wouldn't that cause some confusion? Consider the following sequence:
fd = open("/dev/tee0"); ioctl(fd, TEE_IOC_OPEN, &arg);
What would be the intuitive interpretation of TEE_IOC_OPEN in this sequence? I would prefer keeping the _session suffix here to be clear that it's a session we're starting and ending. It doesn't have to be "_session", anything which makes it obvious that it has nothing to do with open(2) and close(2) is fine.
Ok. I am generally reluctant to maintain GP’s terminology, but I see your point.
For invoke and cancel it makes sense to shorten them to TEE_IOC_INVOKE and TEE_IOC_CANCEL. We should avoid send as it implies that it could be asynchronous, which it isn’t.
You are right, invoke is a better name for sync. communications. I have seen asynchronous TA’s in other contexts, but it is not relevant as long as we only support OP-TEE and Open Virtualization.
We'll have to add two IOCs for tee-supplicant also: struct tee_suppl_arg { union { __u32 func; __u32 ret; } u; __u32 num_params; }; TEE_IOC_SUPPL_RECV TEE_IOC_SUPPL_SEND
Both followed by a struct msg_param in the same way as for open_session and invoke above. u.func is used by RECV and u.ret is used by SEND
+1
If supplicant serves with several threads it should open the device once for each thread. One particular TEE_IOC_SUPPL_RECV on a file descriptor should always be followed by one TEE_IOC_SUPPL_SEND. The driver may refuse opening the device several times if it doesn't support multi-threading.
+1 This should be the same for kernel space.
Also, we have seen that shared memory operations are common to the TEEs we have worked with, so I would suggest to add them to the interface too.
Those are already there, in include/uapi/linux/tee.h
Yes. What I meant is that they should stay there; it is “add” to the commands you mentioned above (tee_cancel_command_arg, tee_invoke_command_arg , etc.)
The tee_open_session_arg and tee_invoke_command_arg structs would be followed by a struct msg_param as defined in optee_msg.h in patch v3 https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to: #define TEE_IMPL_ID_OPTEE 1 #define TEE_OPTEE_CAP_TZ (1 << 0) struct tee_ioctl_version_data { __u32 impl_id; __u32 capabilities; }; or equivalent.
Advantages:
- User space gets a well defined interface
- It will be easier to create a uniform API for the coming kernel internal
API 3. The subsystem could offload more from the driver.
+1
Disadvantages:
- Risk that we'll need to extend the API if a field is missing for some
TEE.
We should be able to define read/write/send pretty good to start with - they are not that complicated. Then we can add more functionality to the interface as we define new trusted services that we want to expose to kernel submodules and user space.
+1
I'd like some feedback on providing a well defined user space API. Note that the user space API shouldn't enforce a certain protocol between normal and secure world.
As I mentioned I think that treating user and kernel space separately is an error, since we risk to end up with a fragmented interface (this is indeed one of the comments we got regarding kernel subsystems). I would send a revision of the driver where the kernel interface is explicit and ioctls map to them (after converting to kernel memory space, etc.).
Agree, I'm thinking that the specific driver will provide functions in struct tee_driver_ops that matches the TEE_IOC_{OPEN_SESSION,CLOSE_SESSION,INVOKE,CANCEL}. Those functions could then be reused in some way for the kernel interface too.
Exactly.
It looks like Javier and I agrees here.
I will make a new patch set based on this and send it to this list first to give you a chance to give feedback before I send PATCH V4 to the kernel mailing lists .
-- Regards, Jens
Hi Jens,
On Thu, Jun 11, 2015 at 01:21:31PM +0200, Jens Wiklander wrote:
It looks like Javier and I agrees here.
I'm also OK with this approach. I did however like the idea with having an opaque command. But I also see the point in using a more strictly defined interface.
One thing that bothers me which I don't think we can do much about is that we're having both ret and ret_origin. For OP-TEE which uses GP specified return values it's hard to do anything about it. But I wish they had set a side a few bits (2 or 3) for origin and the rest could be used for the real return value. However, as I said I don't think there is much to do about it right now. It's too late basically.
I will make a new patch set based on this and send it to this list first to give you a chance to give feedback before I send PATCH V4 to the kernel mailing lists .
-- Regards, Jens
-
Javier González -
Jens Wiklander -
Joakim Bech