| Message ID | 1484744296-30003-1-git-send-email-jens.wiklander@linaro.org |
|---|---|
| Headers | show |
| Series | generic TEE subsystem | expand |
On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote: > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote: > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote: > > > On Wednesday, January 18, 2017 1:58:14 PM CET Jens Wiklander wrote: > > > > > +static int __init optee_driver_init(void) > > > > +{ > > > > + struct device_node *node; > > > > + > > > > + /* > > > > + * Preferred path is /firmware/optee, but it's the matching that > > > > + * matters. > > > > + */ > > > > + for_each_matching_node(node, optee_match) > > > > + of_platform_device_create(node, NULL, NULL); > > > > + > > > > + return platform_driver_register(&optee_driver); > > > > +} > > > > +module_init(optee_driver_init); > > > > + > > > > +static void __exit optee_driver_exit(void) > > > > +{ > > > > + platform_driver_unregister(&optee_driver); > > > > +} > > > > +module_exit(optee_driver_exit); > > > > > > What is the platform driver good for if the same module has to create the > > > platform devices itself? > > > > The platform device(s) are created here because the optee node is below > > "/firmware" instead of the root where it would have had the platform > > device created automatically. > > > > I think it's useful to be able to unload the module, the early reviews > > of this patch set was much focused around that. Regardless I'll need > > some device as parent for the devices created during optee_probe() and > > using a platform device for that seems natural. > > > > I'd rather keep the platform driver. Perhaps some variant of the pattern > > in qcom_scm_init() (drivers/firmware/qcom_scm.c) is useful, except that > > I need to find out what to do about the life cycle of the objects > > created with of_platform_populate(). > > My point was that I don't think we need devices here at all. It's different > when you talk to external hardware that has register resource etc that > can be best abstracted as a real device, but for other firmware features > we don't normally add one. > > Module unloading can also be done without the device. > > > > > > > I'd just skip it and do > > > > > > for_each_matching_node(node, optee_match) > > > optee_probe(node); > > > > > > I also suspect that module unloading is broken here if you don't clean > > > up the platform devices in the end, so you should already remove the > > > exit function to prevent unloading. > > > > Does the platform devices really need cleaning? I mean > > of_platform_default_populate_init() creates a bunch of platform devices > > which are just left there even if unused. Here we're doing the same > > thing except that we're doing it for a specific node in the DT. > > I think it will work if you don't clean them up, but it feels wrong > to have a loadable module that creates devices when loaded but doesn't > remove them when unloaded. > > This could be done differently by having the device creation done in > one driver and the the user of that device in another driver, but I > think just killing off the device achieves the same in a simpler way. I see your point. My final concern here is that with device we got entries in sysfs and uevents that could be used to automatically start the correct supplicant. Different drivers are likely to require different supplicants. Starting the correct supplicant based on uevents is a quite elegant solution which I'm not sure how to support when skipping devices. Perhaps I could create an object below <sysfs>/firmware/tee ? > > > > > +/* > > > > + * Get revision of Trusted OS. > > > > + * > > > > + * Used by non-secure world to figure out which version of the Trusted OS > > > > + * is installed. Note that the returned revision is the revision of the > > > > + * Trusted OS, not of the API. > > > > + * > > > > + * Returns revision in 2 32-bit words in the same way as > > > > + * OPTEE_MSG_CALLS_REVISION described above. > > > > + */ > > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MAJOR 1 > > > > +#define OPTEE_MSG_OS_OPTEE_REVISION_MINOR 0 > > > > +#define OPTEE_MSG_FUNCID_GET_OS_REVISION 0x0001 > > > > > > Just for my understanding, what is the significance of these numbers, > > > i.e. which code (user space, kernel driver, trusted OS) provides > > > the uuid and which one provides the version? The code comments almost > > > make sense to me, but I don't see why specific versions are listed > > > in this header. > > > > You're right, OPTEE_MSG_OS_OPTEE_REVISION_* should be removed. The > > actual version the secure OS is of a mostly informational nature. The > > same goes the OS UUID, but I suppose the actual UUID used by the > > upstream version of OP-TEE OS could be interesting to know. > ... > > > What is the expected behavior when one side reports a version that > > > is unknown? Can one side claim to be backwards compatible with > > > a previous version, or does each new version need support on > > > all three sides? > > > > The UUID and version of the message protocol are important to match > > correctly as otherwise it could mean that there's something unexpected > > in secure world that following the message protocol would be undefined > > behaviour. All changes to the message protocol should be backwards > > compatible in the sense that the driver and secure world need to > > negotiate eventual extensions while probing. That's what we're doing in > > optee_msg_exchange_capabilities(). > > Ok, then maybe the "compatible" identifier in DT should be sufficient > to ensure that the capability exchange works, and the rest be based > on that? > > We tend to avoid version checks for APIs in the kernel because they > never work in practice, but the capability check should be fine. UUID and version of the message protocol is required by ARM SMC Calling Convention. It will be there anyway so we could just as well check it in the probe function to catch eventual mismatches in configuration. Since we're using capabilities to manage extensions of the protocol I think the minor version could be ignored by probe. Thanks, Jens
On Mon, Jan 23, 2017 at 05:16:15PM +0100, Arnd Bergmann wrote: > On Monday, January 23, 2017 10:08:53 AM CET Jens Wiklander wrote: > > On Fri, Jan 20, 2017 at 05:57:51PM +0100, Arnd Bergmann wrote: > > > On Thursday, January 19, 2017 3:56:23 PM CET Jens Wiklander wrote: > > > > On Wed, Jan 18, 2017 at 05:28:17PM +0100, Arnd Bergmann wrote: > > > > > Does the platform devices really need cleaning? I mean > > > > of_platform_default_populate_init() creates a bunch of platform devices > > > > which are just left there even if unused. Here we're doing the same > > > > thing except that we're doing it for a specific node in the DT. > > > > > > I think it will work if you don't clean them up, but it feels wrong > > > to have a loadable module that creates devices when loaded but doesn't > > > remove them when unloaded. > > > > > > This could be done differently by having the device creation done in > > > one driver and the the user of that device in another driver, but I > > > think just killing off the device achieves the same in a simpler way. > > > > I see your point. My final concern here is that with device we got > > entries in sysfs and uevents that could be used to automatically start > > the correct supplicant. Different drivers are likely to require > > different supplicants. Starting the correct supplicant based on uevents > > is a quite elegant solution which I'm not sure how to support when > > skipping devices. Perhaps I could create an object below > > <sysfs>/firmware/tee ? > > Putting the objects somewhere other than /sys/devices sounds good, yes. > This would also help with TEE implementations that might get probed > differently. > > I think the natural place would be /sys/class/tee/, as we normally > require something in /sys/class anyway to support the character > device. > > /sys/firmware/tee/ sounds less fitting, as there other TEE implementations > are not necessarily firmware based, as you point out. > /sys/firmware/op-tee certainly makes sense for anything that is specific > to OP-TEE in particular, while /sys/class/tee would be for anything > that uses the ioctl interface. This part is particularly important to > get right from the start, just like the ioctls themselves we can't make > incompatible changes here later once there are users relying on the > upstream kernel interfaces. /sys/class/tee/ sounds good, I'll use that. It's more or less what we also have today. Thanks for the help with this review. Jens
Hi, This patch set introduces a generic TEE subsystem. These patches are used on HiKey in AOSP. It's currently supported by roughly 20 platforms (https://github.com/OP-TEE/optee_os#3-platforms-supported), to name a few, Renesas RCAR H3, Sequitur Labs CoreTEE, Wind River VxWorks etc. I’m not sure how to progress this. I’ve had only positive feedback on the previous v12 version. In the previous versions I’ve addressed all feedback (which has been getting more and more minor) and I haven’t had any response from Greg since February. Is there anything else we can be doing here to help progress this? The TEE subsystem will contain drivers for various TEE implementations. A TEE (Trusted Execution Environment) is a trusted OS running in some secure environment, for example, TrustZone on ARM CPUs, or a separate secure co-processor etc. Regarding use cases, TrustZone has traditionally been used for offloading secure tasks to the secure world. Examples include: - Secure key handling where the OS may or may not have direct access to key material. - E-commerce and payment technologies. Credentials, credit card numbers etc could be stored in a more secure environment. - Trusted User Interface (TUI) to ensure that no-one can snoop PIN-codes etc. - Secure boot to ensure that loaded binaries haven’t been tampered with. It’s not strictly needed for secure boot, but you could enhance security by leveraging a TEE during boot. - Digital Rights Management (DRM), the studios provides content with different resolution depending on the security of the device. Higher security means higher resolution. A TEE could also be used in existing and new technologies. For example IMA (Integrity Measurement Architecture) which has been in the kernel for quite a while. Today you can enhance security by using a TPM-chip to sign the IMA measurement list. This is something that also has been done in practical systems by leveraging a TEE. Another example could be in 2-factor authentication which is becoming increasingly more important. FIDO (https://fidoalliance.org) for example are using public key cryptography in their 2-factor authentication standard (U2F). With FIDO, a private and public key pair will be generated for every site you visit and the private key should never leave the local device. This is an example where you could use secure storage in a TEE for the private key. Today you will find a quite a few different out of tree implementations of TEE drivers which tends to fragment the TEE ecosystem and development. We think it would be a good idea to have a generic TEE driver integrated in the kernel which would serve as a base for several different TEE solutions, no matter if they are on-chip like TrustZone or if they are on a separate crypto co-processor. To develop this TEE subsystem we have been using the open source TEE called OP-TEE (https://www.op-tee.org/) and therefore this would be the first TEE solution supported by this new subsystem. OP-TEE is a GlobalPlatform compliant TEE, however this TEE subsystem is not limited to only GlobalPlatform TEEs, instead we have tried to design it so that it should work with other TEE solutions also. Since the first version (2015 April) of this patchset we’ve talked about it at Linaro Connect, GlobalPlatform annual TEE conference and last time at Linux Plumbers, so we think there has been both exposure and plenty of time to be able to get involved in this work. "tee: generic TEE subsystem" brings in the generic TEE subsystem which helps when writing a driver for a specific TEE, for example, OP-TEE. "tee: add OP-TEE driver" is an OP-TEE driver which uses the subsystem to do its work. This patch set has been prepared in cooperation with Javier González who proposed "Generic TrustZone Driver in Linux Kernel" patches 28 Nov 2014, https://lwn.net/Articles/623380/ . We've since then changed the scope to TEE instead of TrustZone. We have discussed the design on tee-dev@lists.linaro.org (archive at https://lists.linaro.org/pipermail/tee-dev/) with people from other companies, including Valentin Manea <valentin.manea@huawei.com>, Emmanuel MICHEL <emmanuel.michel@st.com>, Jean-michel DELORME <jean-michel.delorme@st.com>, and Joakim Bech <joakim.bech@linaro.org>. Our main concern has been to agree on something that is generic enough to support many different TEEs while still keeping the interface together. v14: * Rebased on v4.10-rc4 * Fixed checkpatch warning in OPTEE_SMC_RETURN_IS_RPC() macro v13: * Rebased on v4.9-rc5 * Added Hikey DT patch * Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey) * Tested-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> (RCAR H3) * Tested-by: Andrew F. Davis <afd@ti.com> v12-resend: * Rebased on v4.9-rc2 v12: * Rebased on v4.8-rc5 * Addressed review comments from Andrew F. Davis * Removed Acked-by: Andreas Dannenberg <dannenberg@ti.com> as the mail bounces * Bugfix possible null dereference in error cleanup path of optee_probe(). * Bugfix optee_from_msg_param() when calculating offset of memref into a shared memory object v11: * Rebased on v4.8-rc3 * Addressed review comments from Nishanth Menon * Made the TEE framework available as a loadable module. * Reviewed-by: Javier González <javier@javigon.com> * Zeroes shared memory on allocation to avoid information leakage * Links shared memory objects to context to avoid stealing of shared memory object from an unrelated process * Allow RPC interruption if supplicant is unavailable v10: * Rebased on v4.7-rc1 * Addressed private review comments from Nishanth Menon * Optee driver only accepts one supplicant process on the privileged device * Optee driver avoids long delayed releases of shm objects * Added more comments on functions and structs v9: * Rebased on v4.6-rc1 * Acked-by: Andreas Dannenberg <dannenberg@ti.com> * Addressed comments from Al Viro how file descriptors are passed to user space * Addressed comments from Randy Dunlap on documentation * Changed license for include/uapi/linux/tee.h v8: * Rebased on v4.5-rc3 * dt/bindings: add bindings for optee Acked-by: Rob Herring <robh@kernel.org> * Fixes build error for X86 * Fixes spell error in "dt/bindings: add bindings for optee" v7: * Rebased on v4.5-rc2 * Moved the ARM SMC Calling Convention support into a separate patch set, which is now merged v6: * Rebased on v4.3-rc7 * Changed smccc interface to let the compiler marshal most of the parameters * Added ARCH64 capability for smccc interface * Changed the PSCI firmware calls (both arm and arm64) to use the new generic smccc interface instead instead of own assembly functions. * Move optee DT bindings to below arm/firmware * Defines method for OP-TEE driver to call secure world in DT, smc or hvc * Exposes implementation id of a TEE driver in sysfs to easily spawn corresponding tee-supplicant when device is ready * Update OP-TEE Message Protocol to better cope with fragmented physical memory * Read time directly from OP-TEE driver instead of forwarding the RPC request to tee-supplicant v5: * Replaced kref reference counting for the device with a size_t instead as the counter is always protected by a mutex v4: * Rebased on 4.1 * Redesigned the synchronization around entry exit of normal SMC * Replaced rwsem on the driver instance with kref and completion since rwsem wasn't intended to be used in this way * Expanded the TEE_IOCTL_PARAM_ATTR_TYPE_MASK to make room for future additional parameter types * Documents TEE subsystem and OP-TEE driver * Replaced TEE_IOC_CMD with TEE_IOC_OPEN_SESSION, TEE_IOC_INVOKE, TEE_IOC_CANCEL and TEE_IOC_CLOSE_SESSION * DT bindings in a separate patch * Assembly parts moved to arch/arm and arch/arm64 respectively, in a separate patch * Redefined/clarified the meaning of OPTEE_SMC_SHM_CACHED * Removed CMA usage to limit the scope of the patch set v3: * Rebased on 4.1-rc3 (dma_buf_export() API change) * A couple of small sparse fixes * Documents bindings for OP-TEE driver * Updated MAINTAINERS v2: * Replaced the stubbed OP-TEE driver with a real OP-TEE driver * Removed most APIs not needed by OP-TEE in current state * Update Documentation/ioctl/ioctl-number.txt with correct path to tee.h * Rename tee_shm_pool_alloc_cma() to tee_shm_pool_alloc() * Moved tee.h into include/uapi/linux/ * Redefined tee.h IOCTL macros to be directly based on _IOR and friends * Removed version info on the API to user space, a data blob which can contain an UUID is left for user space to be able to tell which protocol to use in TEE_IOC_CMD * Changed user space exposed structures to only have types with __ prefix * Dropped THIS_MODULE from tee_fops * Reworked how the driver is registered and ref counted: - moved from using an embedded struct miscdevice to an embedded struct device. - uses an struct rw_semaphore as synchronization for driver detachment - uses alloc/register pattern from TPM Thanks, Jens Jens Wiklander (4): dt/bindings: add bindings for optee tee: generic TEE subsystem tee: add OP-TEE driver Documentation: tee subsystem and op-tee driver Jerome Forissier (1): arm64: dt: hikey: Add optee node Documentation/00-INDEX | 2 + .../bindings/arm/firmware/linaro,optee-tz.txt | 31 + .../devicetree/bindings/vendor-prefixes.txt | 1 + Documentation/ioctl/ioctl-number.txt | 1 + Documentation/tee.txt | 118 +++ MAINTAINERS | 13 + arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 7 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/tee/Kconfig | 18 + drivers/tee/Makefile | 5 + drivers/tee/optee/Kconfig | 7 + drivers/tee/optee/Makefile | 5 + drivers/tee/optee/call.c | 435 ++++++++++ drivers/tee/optee/core.c | 598 ++++++++++++++ drivers/tee/optee/optee_msg.h | 435 ++++++++++ drivers/tee/optee/optee_private.h | 185 +++++ drivers/tee/optee/optee_smc.h | 450 ++++++++++ drivers/tee/optee/rpc.c | 404 +++++++++ drivers/tee/optee/supp.c | 273 +++++++ drivers/tee/tee_core.c | 901 +++++++++++++++++++++ drivers/tee/tee_private.h | 129 +++ drivers/tee/tee_shm.c | 357 ++++++++ drivers/tee/tee_shm_pool.c | 158 ++++ include/linux/tee_drv.h | 278 +++++++ include/uapi/linux/tee.h | 401 +++++++++ 26 files changed, 5215 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt create mode 100644 Documentation/tee.txt create mode 100644 drivers/tee/Kconfig create mode 100644 drivers/tee/Makefile create mode 100644 drivers/tee/optee/Kconfig create mode 100644 drivers/tee/optee/Makefile create mode 100644 drivers/tee/optee/call.c create mode 100644 drivers/tee/optee/core.c create mode 100644 drivers/tee/optee/optee_msg.h create mode 100644 drivers/tee/optee/optee_private.h create mode 100644 drivers/tee/optee/optee_smc.h create mode 100644 drivers/tee/optee/rpc.c create mode 100644 drivers/tee/optee/supp.c create mode 100644 drivers/tee/tee_core.c create mode 100644 drivers/tee/tee_private.h create mode 100644 drivers/tee/tee_shm.c create mode 100644 drivers/tee/tee_shm_pool.c create mode 100644 include/linux/tee_drv.h create mode 100644 include/uapi/linux/tee.h -- 2.7.4