Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Both tee and teepriv reported GlobalPlatform compliant #1199

Closed
msa2 opened this issue Nov 25, 2016 · 17 comments
Closed

Both tee and teepriv reported GlobalPlatform compliant #1199

msa2 opened this issue Nov 25, 2016 · 17 comments

Comments

@msa2
Copy link

msa2 commented Nov 25, 2016

Both teepriv0 and tee0 devices report in tee_ioctl_version_data.gen_caps as GlobalPlatform compliant devices (TEE_GEN_CAP_GP).

Maybe I misunderstand something, but to me "teepriv0" is not GP compliant or is it?

The question comes up, when I try to select GP compliant TEE device (without needing to hardcode "tee0" vs "teepriv0" difference? Or, would it make difference? Can client use either as GP compliant tee device?

@etienne-lms
Copy link
Contributor

"teepriv0" is dedicated to the TEE supplicant. It is dedicated to optee remote (nonsecure) services. Hence it is not designed for GP TEE client applications.

GP TEE Client Application shall use the "/dev/tee0" device.
The "TEE Client API library" (libteec provided by optee_client), opens this "/dev/tee0". (actually, it scans all "/dev/teeN", from N=0 to N=9, to look for a "TEE_GEN_CAP_GP" compliant device).

Note that "libteec" provides a "GP TEE Client API" compliant interface while the "/dev/tee*" devices provides ioctl (and other) interfaces obviously not following GP TEE Client API specifications. GP TEE Client Application should use the "libteec" rather than issuing straight ioctls to a "/dev/teeXXX" device.

@msa2
Copy link
Author

msa2 commented Nov 28, 2016

I must resurrect the kernel client API, and for that work I was looking at

https://github.com/linaro-swg/linux/commit/b8d05a0934b2

which in match_dev is basing selection purely on tee_ioctl_version_data and what this issue is about, is that the version data does not now differentiate between the two devices. I suppose one could modify the code to find and test "TEE_DESC_PRIVILEGED" flag, and skip those devices.

Also, I would prefer an API that closer to existing GP specification, like the old driver

https://github.com/OP-TEE/optee_linuxdriver

so, I'm probably going to hack something from those two existing works.

@etienne-lms
Copy link
Contributor

etienne-lms commented Nov 28, 2016

Mainly due to coding style issues, the "GP TEE CLient API" as specifiy by GP Device will never hit the linux kernel land. The "new kernel client API" you refer to (linaro-swg/linux@b8d05a0934b2) allows to invoke op-tee services through a linux styled API.

You can have a look here etienne-lms/linux@f698b6807544 to see an example of a linux driver calling the op-tee kernel API you resurrected. Leave aside all the 'smaf' stuff of it and check only the tee_client_xxx() and tee_shm_alloc/_free() calls.

Note that the match routine (use to open the tee context) simply returns true as this driver does not expects several tee devices in the system. It is indeed a shortcut.

In the hope it may help....

@msa2
Copy link
Author

msa2 commented Nov 28, 2016

So you are using the "new kernel client API" as is. I suppose I can base my on top of it also, but it would be nice if it was already included into main line, and not forcing everyone do cherry-pick and manual conflict fixing.

@etienne-lms
Copy link
Contributor

Actually it would be even nicer if the 'tee driver' was in the mainline...
Our apologies on that but we expect the tee driver to hit the kernel first. Once it's done (and driver is fully stabilized regarding linux kernel standards) then such add-ons would come once they are matured enough.

To answer your question: yes, we (at linaro-swg) do use this new API as is, at least for those who need such a 'TEE' kernel api.

@jbech-linaro
Copy link
Contributor

@msa2 , so do we think and we've been trying to get it mainlined for quite a while now (first patch sent March/April 2015) to LKML).
https://patchwork.kernel.org/project/linux-arm-kernel/list/?submitter=129291

Our major challenge is that the current kernel maintainers doesn't understand the technology and the users of OP-TEE and this driver hasn't been very active on reviewing and adding feedback to the patches. This rather long ride has so far unfortunately been more headache for us than we've gained by trying get it upstreamed. For example we have a bunch of different branches and they aren't exactly the same everywhere. If we cannot get anywhere with this merged soon, then eventually we will re-consider the strategy and it might be that out-of-tree driver is an option yet again.

@msa2
Copy link
Author

msa2 commented Nov 29, 2016

Returning back to issue...

Note that the match routine (use to open the tee context) simply returns true as this driver does not expects several tee devices in the system. It is indeed a shortcut.

But, optee does have two devices 'tee0' and 'teepriv0', and looks like it's just lucky coincidence that 'tee0' is tested first, and returned?

Also, I think the callback match routine should get more than just the version struct, the dev or teedev could be considered?

Anyway, if people think this is no issue, you can close this then.

@jforissier
Copy link
Contributor

FWIW, like you @msa2 I don't think it's OK to have teeprivX identify exactly as teeX.

@jenswi-linaro
Copy link
Contributor

@jforissier, what do you suggest given that the current ABI shouldn't have incompatible changes?
I suppose mirroring the TEE_DESC_PRIVILEGED bit of struct tee_desc:flags into struct tee_ioctl_version_data:gen_caps as TEE_GEN_CAP_PRIVILEGED in one way.

@jforissier
Copy link
Contributor

@jenswi-linaro sounds good IMO.

@msa2
Copy link
Author

msa2 commented Nov 30, 2016

This is not directly related to this issue, but as github does not seem to have private messages anymore, I hopefully ask a question here:

  • I succesfully added the "kernel api" to tee_core.c
  • ctx = tee_client_open_context returns success
  • tee_client_open_session returns success (for hello world TA)
  • shm_buf = tee_shm_alloc(ctx, 4096, TEE_SHM_MAPPED) returns -12 (ENOMEM). Any idea what might be wrong?

@msa2
Copy link
Author

msa2 commented Dec 1, 2016

... but

  • shm_buf = tee_shm_alloc(ctx, 4096, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);

succeeds. But I don't need DMA buffer. Actually, it would be much easier if I could just pass a kernel address to TEE without SHM juggling (some kind of "fake" shm that maps the kernel address space :-)

@jenswi-linaro
Copy link
Contributor

Without the TEE_SHM_DMA_BUF flag the memory is allocated from a very small pool designed for keeping the struct optee_msg_arg.

Shared memory here isn't just memory shared with user space, it's also memory shared with secure world and secure world has some restrictions.

@jbech-linaro
Copy link
Contributor

jbech-linaro commented Feb 15, 2017

@msa2 @jenswi-linaro @jforissier After re-reading the discussion it seems like we have a proposal, but not still implemented fix for teeX / teeprivX? Right? Jens, is this something that you could/should pick up in the generic TEE patches?

@jenswi-linaro
Copy link
Contributor

I've created linaro-swg/linux#37 to take care of this.

@jbech-linaro
Copy link
Contributor

Thanks for the quick turn around time since yesterday Jens. Since the issue has been addressed, I'm going to close it.

@ghost
Copy link

ghost commented Feb 16, 2017

We're closing this issue since the question has been answered. If you however feel that you have additional questions or still thinks this is an issue, please feel free to re-open the issue again.

@ghost ghost closed this as completed Feb 16, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants