Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411177 > unrolled thread

[PATCH v10 0/4] generic TEE subsystem

Started byJens Wiklander <jens.wiklander@linaro.org>
First post2016-06-01 14:50 +0200
Last post2016-06-07 14:00 +0200
Articles 7 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v10 0/4] generic TEE subsystem Jens Wiklander <jens.wiklander@linaro.org> - 2016-06-01 14:50 +0200
    Re: [PATCH v10 0/4] generic TEE subsystem Javier González <javier@javigon.com> - 2016-06-06 21:00 +0200
    Re: [PATCH v10 2/4] tee: generic TEE subsystem Nishanth Menon <nm@ti.com> - 2016-06-06 23:50 +0200
      Re: [PATCH v10 2/4] tee: generic TEE subsystem Jens Wiklander <jens.wiklander@linaro.org> - 2016-06-07 13:00 +0200
        Re: [PATCH v10 2/4] tee: generic TEE subsystem Joe Perches <joe@perches.com> - 2016-06-07 16:40 +0200
    Re: [PATCH v10 3/4] tee: add OP-TEE driver Nishanth Menon <nm@ti.com> - 2016-06-07 00:00 +0200
      Re: [PATCH v10 3/4] tee: add OP-TEE driver Jens Wiklander <jens.wiklander@linaro.org> - 2016-06-07 14:00 +0200

#1411177 — [PATCH v10 0/4] generic TEE subsystem

FromJens Wiklander <jens.wiklander@linaro.org>
Date2016-06-01 14:50 +0200
Subject[PATCH v10 0/4] generic TEE subsystem
Message-ID<rFdKW-8sS-5@gated-at.bofh.it>
Hi,

[TL;DR; This patch set needs more review, if you're using OP-TEE please
help reviewing.]

This patch set introduces a generic TEE subsystem. 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 you also could do 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://github.com/OP-TEE/optee_os) 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.

"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.

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

 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 +
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/tee/Kconfig                                |  19 +
 drivers/tee/Makefile                               |   4 +
 drivers/tee/optee/Kconfig                          |   8 +
 drivers/tee/optee/Makefile                         |   5 +
 drivers/tee/optee/call.c                           | 422 ++++++++++
 drivers/tee/optee/core.c                           | 553 +++++++++++++
 drivers/tee/optee/optee_msg.h                      | 435 ++++++++++
 drivers/tee/optee/optee_private.h                  | 181 +++++
 drivers/tee/optee/optee_smc.h                      | 418 ++++++++++
 drivers/tee/optee/rpc.c                            | 401 ++++++++++
 drivers/tee/optee/supp.c                           | 241 ++++++
 drivers/tee/tee.c                                  | 877 +++++++++++++++++++++
 drivers/tee/tee_private.h                          | 125 +++
 drivers/tee/tee_shm.c                              | 347 ++++++++
 drivers/tee/tee_shm_pool.c                         | 155 ++++
 include/linux/tee_drv.h                            | 273 +++++++
 include/uapi/linux/tee.h                           | 400 ++++++++++
 25 files changed, 5033 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.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

-- 
1.9.1

[toc] | [next] | [standalone]

#1415378

FromJavier González <javier@javigon.com>
Date2016-06-06 21:00 +0200
Message-ID<rH7UK-da-25@gated-at.bofh.it>
In reply to#1411177

[Multipart message — attachments visible in raw view] — view raw

Hi,

> On 01 Jun 2016, at 14:41, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> [TL;DR; This patch set needs more review, if you're using OP-TEE please
> help reviewing.]
> 
> This patch set introduces a generic TEE subsystem. 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.

Have you considered moving the code to a security subsystem? Back in the
days I talked to some of the TPM maintainers and they were not closed to
the idea of having support for hardware security extensions in a single
place. I think TPM is still considered as a char device...

We would all benefit if it were possible to have an overview of all
available all the security extensions, if nothing else at least for the
documentation.

Javier

[toc] | [prev] | [next] | [standalone]

#1415504 — Re: [PATCH v10 2/4] tee: generic TEE subsystem

FromNishanth Menon <nm@ti.com>
Date2016-06-06 23:50 +0200
SubjectRe: [PATCH v10 2/4] tee: generic TEE subsystem
Message-ID<rHazf-1ZA-11@gated-at.bofh.it>
In reply to#1411177
On 06/01/2016 07:41 AM, Jens Wiklander wrote:
few minor comments below.

I see the patch generated (with --strict):
> CHECK: Alignment should match open parenthesis
> #512: FILE: drivers/tee/tee.c:375:
> +static int tee_ioctl_close_session(struct tee_context *ctx,
> +              struct tee_ioctl_close_session_arg __user *uarg)
> CHECK: Alignment should match open parenthesis
> #1607: FILE: drivers/tee/tee_shm_pool.c:103:
> +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
> +                      struct tee_shm_pool_mem_info *priv_info,
> CHECK: Alignment should match open parenthesis
> #1789: FILE: include/linux/tee_drv.h:124:
> +struct tee_device *tee_device_alloc(const struct tee_desc *teedesc,
> +                      struct device *dev, struct tee_shm_pool *pool,
> WARNING: line over 80 characters
> #1814: FILE: include/linux/tee_drv.h:149:
> + * struct tee_shm_pool_mem_info - holds information needed to create a shared memory pool  
> WARNING: line over 80 characters
> #1826: FILE: include/linux/tee_drv.h:161:
> + * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved memory range  
> CHECK: Alignment should match open parenthesis
> #1839: FILE: include/linux/tee_drv.h:174:
> +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,                        
> +                      struct tee_shm_pool_mem_info *priv_info,
> CHECK: Prefer using the BIT macro
> #1995: FILE: include/uapi/linux/tee.h:51:
> +#define TEE_GEN_CAP_GP                (1 << 0)/* GlobalPlatform compliant TEE */ 
> CHECK: Prefer using the BIT macro
> #2005: FILE: include/uapi/linux/tee.h:61:
> +#define TEE_OPTEE_CAP_TZ      (1 << 0)
> WARNING: line over 80 characters
> #2205: FILE: include/uapi/linux/tee.h:261:
> + * struct tee_ioctl_invoke_func_arg - Invokes a function in a Trusted Application 
>       mechanically convert to the typical style using --fix or --fix-inplace.
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.

Might be nice to fix them up.

[...]

> diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> new file mode 100644
> index 0000000..f3ba154
> --- /dev/null
> +++ b/drivers/tee/Kconfig
> @@ -0,0 +1,9 @@
> +# Generic Trusted Execution Environment Configuration
> +config TEE
> +	bool "Trusted Execution Environment support"

Why could not this be tristate? we would like to enforce subsystem init?

> +	default n

You should not need this. (default is n)

> +	select DMA_SHARED_BUFFER
> +	select GENERIC_ALLOCATOR

select or depends?

> +	help
> +	  This implements a generic interface towards a Trusted Execution
> +	  Environment (TEE).
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> new file mode 100644
> index 0000000..60d2dab
> --- /dev/null
> +++ b/drivers/tee/Makefile
> @@ -0,0 +1,3 @@
> +obj-y += tee.o
> +obj-y += tee_shm.o
> +obj-y += tee_shm_pool.o
> diff --git a/drivers/tee/tee.c b/drivers/tee/tee.c
> new file mode 100644
> index 0000000..119e18e
> --- /dev/null
> +++ b/drivers/tee/tee.c
> @@ -0,0 +1,877 @@
> +/*
> + * Copyright (c) 2015-2016, Linaro Limited
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
Adding a
#define pr_fmt(fmt) "%s: " fmt, __func__ might help
might help give reasonable errors where pr_* is used.

> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/slab.h>
> +#include <linux/tee_drv.h>
> +#include <linux/uaccess.h>
> +#include "tee_private.h"
> +
> +#define TEE_NUM_DEVICES	32

I have a personal allergy to MAX_* macros, so I wonder if idr can help
us get rid of fixed size tables? I wonder if the use should be limited
to tee_shm.c ?

static DEFINE_IDR(tee_id);
....

teedev->id =  idr_alloc(&tee_id, teedev, 0, 0, GFP_KERNEL);
or something similar?

> +
> +#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x))
> +
> +/*
> + * Unprivileged devices in the in the lower half range and privileged
> + * devices in the upper half range.
> + */
> +static DECLARE_BITMAP(dev_mask, TEE_NUM_DEVICES);
> +static DEFINE_SPINLOCK(driver_lock);

I think you might be able to get rid of the above two with idr usage.

> +
> +static struct class *tee_class;
> +static dev_t tee_devt;
> +
> +static int tee_open(struct inode *inode, struct file *filp)
> +{
> +	int rc;
> +	struct tee_device *teedev;
> +	struct tee_context *ctx;
> +
> +	teedev = container_of(inode->i_cdev, struct tee_device, cdev);
> +	if (!tee_device_get(teedev))
> +		return -EINVAL;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ctx->teedev = teedev;
> +	filp->private_data = ctx;

I wonder if the teedev was a module, could it be removed /
unregistered after tee_open was invoked?

> +static int tee_ioctl_invoke(struct tee_context *ctx,
> +			    struct tee_ioctl_buf_data __user *ubuf)
> +{
> +	int rc;
> +	size_t n;
> +	struct tee_ioctl_buf_data buf;
> +	struct tee_ioctl_invoke_arg __user *uarg;
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_ioctl_param __user *uparams = NULL;
> +	struct tee_param *params = NULL;
> +
> +	if (!ctx->teedev->desc->ops->invoke_func)
> +		return -EINVAL;
> +
> +	rc = copy_from_user(&buf, ubuf, sizeof(buf));
> +	if (rc)
> +		return rc;
> +
> +	if (buf.buf_len > TEE_MAX_ARG_SIZE ||
> +	    buf.buf_len < sizeof(struct tee_ioctl_invoke_arg))
> +		return -EINVAL;
> +
> +	uarg = (struct tee_ioctl_invoke_arg __user *)(unsigned long)buf.buf_ptr;
> +	if (copy_from_user(&arg, uarg, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len)
> +		return -EINVAL;
> +
> +	if (arg.num_params) {
> +		params = kcalloc(arg.num_params, sizeof(struct tee_param),
> +				 GFP_KERNEL);
> +		if (!params)
> +			return -ENOMEM;
> +		uparams = (struct tee_ioctl_param __user *)(uarg + 1);
> +		rc = params_from_user(ctx, params, arg.num_params, uparams);
> +		if (rc)
> +			goto out;
> +	}
> +
> +	rc = ctx->teedev->desc->ops->invoke_func(ctx, &arg, params);
> +	if (rc)
> +		goto out;

Hmm.. I wonder if the teedev drivers should get subsystem level lock
protection for ops invocation or should they implement locking themselves?

[...]
-- 
Regards,
Nishanth Menon

[toc] | [prev] | [next] | [standalone]

#1415986 — Re: [PATCH v10 2/4] tee: generic TEE subsystem

FromJens Wiklander <jens.wiklander@linaro.org>
Date2016-06-07 13:00 +0200
SubjectRe: [PATCH v10 2/4] tee: generic TEE subsystem
Message-ID<rHmTL-1Cx-9@gated-at.bofh.it>
In reply to#1415504
On Mon, Jun 06, 2016 at 04:44:42PM -0500, Nishanth Menon wrote:
> On 06/01/2016 07:41 AM, Jens Wiklander wrote:
> few minor comments below.
> 
> I see the patch generated (with --strict):
> > CHECK: Alignment should match open parenthesis
> > #512: FILE: drivers/tee/tee.c:375:
> > +static int tee_ioctl_close_session(struct tee_context *ctx,
> > +              struct tee_ioctl_close_session_arg __user *uarg)
> > CHECK: Alignment should match open parenthesis
> > #1607: FILE: drivers/tee/tee_shm_pool.c:103:
> > +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
> > +                      struct tee_shm_pool_mem_info *priv_info,
> > CHECK: Alignment should match open parenthesis

The alternative is to format it as:
struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
                                                struct tee_shm_pool_mem_info
                                                        *priv_info,
                                                struct tee_shm_pool_mem_info
                                                        *dmabuf_info)
But that is a bit awkward. I'd like to keep it as it is if you don't mind.

> > #1789: FILE: include/linux/tee_drv.h:124:
> > +struct tee_device *tee_device_alloc(const struct tee_desc *teedesc,
> > +                      struct device *dev, struct tee_shm_pool *pool,
> > WARNING: line over 80 characters
> > #1814: FILE: include/linux/tee_drv.h:149:
> > + * struct tee_shm_pool_mem_info - holds information needed to create a shared memory pool  
> > WARNING: line over 80 characters
> > #1826: FILE: include/linux/tee_drv.h:161:
> > + * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved memory range  

I'll fix the long lines.

> > CHECK: Alignment should match open parenthesis
> > #1839: FILE: include/linux/tee_drv.h:174:
> > +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,                        
> > +                      struct tee_shm_pool_mem_info *priv_info,
> > CHECK: Prefer using the BIT macro
> > #1995: FILE: include/uapi/linux/tee.h:51:
> > +#define TEE_GEN_CAP_GP                (1 << 0)/* GlobalPlatform compliant TEE */ 
> > CHECK: Prefer using the BIT macro
> > #2005: FILE: include/uapi/linux/tee.h:61:
> > +#define TEE_OPTEE_CAP_TZ      (1 << 0)

This file is included from user space too and BIT is only defined if
__KERNEL__ is defined.

> > WARNING: line over 80 characters
> > #2205: FILE: include/uapi/linux/tee.h:261:
> > + * struct tee_ioctl_invoke_func_arg - Invokes a function in a Trusted Application 
> >       mechanically convert to the typical style using --fix or --fix-inplace.
> >       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> Might be nice to fix them up.
> 
> [...]
> 
> > diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
> > new file mode 100644
> > index 0000000..f3ba154
> > --- /dev/null
> > +++ b/drivers/tee/Kconfig
> > @@ -0,0 +1,9 @@
> > +# Generic Trusted Execution Environment Configuration
> > +config TEE
> > +	bool "Trusted Execution Environment support"
> 
> Why could not this be tristate? we would like to enforce subsystem init?

In the past it wasn't possible, but it is now. I'll fix.

> 
> > +	default n
> 
> You should not need this. (default is n)

I'll fix.

> 
> > +	select DMA_SHARED_BUFFER
> > +	select GENERIC_ALLOCATOR
> 
> select or depends?

These are selected by all the other modules needing these.

> 
> > +	help
> > +	  This implements a generic interface towards a Trusted Execution
> > +	  Environment (TEE).
> > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > new file mode 100644
> > index 0000000..60d2dab
> > --- /dev/null
> > +++ b/drivers/tee/Makefile
> > @@ -0,0 +1,3 @@
> > +obj-y += tee.o
> > +obj-y += tee_shm.o
> > +obj-y += tee_shm_pool.o
> > diff --git a/drivers/tee/tee.c b/drivers/tee/tee.c
> > new file mode 100644
> > index 0000000..119e18e
> > --- /dev/null
> > +++ b/drivers/tee/tee.c
> > @@ -0,0 +1,877 @@
> > +/*
> > + * Copyright (c) 2015-2016, Linaro Limited
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> Adding a
> #define pr_fmt(fmt) "%s: " fmt, __func__ might help
> might help give reasonable errors where pr_* is used.

I'll fix.

> 
> > +#include <linux/cdev.h>
> > +#include <linux/device.h>
> > +#include <linux/fs.h>
> > +#include <linux/idr.h>
> > +#include <linux/slab.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/uaccess.h>
> > +#include "tee_private.h"
> > +
> > +#define TEE_NUM_DEVICES	32
> 
> I have a personal allergy to MAX_* macros, so I wonder if idr can help
> us get rid of fixed size tables? I wonder if the use should be limited
> to tee_shm.c ?
> 
> static DEFINE_IDR(tee_id);
> ....
> 
> teedev->id =  idr_alloc(&tee_id, teedev, 0, 0, GFP_KERNEL);
> or something similar?

That would work, but the we'd need to do something with
alloc_chrdev_region() in tee_init() below also. As long as we're only
doing a single call to alloc_chrdev_region() we'll have an upper limit
of the number of devices and not much if any is gained by using IDR
instead of a fixed sized bit-field.

> 
> > +
> > +#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x))
> > +
> > +/*
> > + * Unprivileged devices in the in the lower half range and privileged
> > + * devices in the upper half range.
> > + */
> > +static DECLARE_BITMAP(dev_mask, TEE_NUM_DEVICES);
> > +static DEFINE_SPINLOCK(driver_lock);
> 
> I think you might be able to get rid of the above two with idr usage.

Yes, but I'd need to add a mutex for synchronization for idr_alloc() and
idr_remove(). To make it easier in user space to tell the privileged
devices apart from the normal client devices I'm using /dev/teeprivX and
/dev/teeX, both are supposed to be numbered from 0 and upwards. Without
a fixed upper limit I would need two IDRs to keep track of the
numbering.

With these obstacles and the one above (alloc_chrdev_region()) in mind
I'd like to keep the fixed MAX number.

> 
> > +
> > +static struct class *tee_class;
> > +static dev_t tee_devt;
> > +
> > +static int tee_open(struct inode *inode, struct file *filp)
> > +{
> > +	int rc;
> > +	struct tee_device *teedev;
> > +	struct tee_context *ctx;
> > +
> > +	teedev = container_of(inode->i_cdev, struct tee_device, cdev);
> > +	if (!tee_device_get(teedev))
> > +		return -EINVAL;
> > +
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx) {
> > +		rc = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	ctx->teedev = teedev;
> > +	filp->private_data = ctx;
> 
> I wonder if the teedev was a module, could it be removed /
> unregistered after tee_open was invoked?

No, even if the shared memory routines was in another module we still
have the tee_class to keep track of.

> 
> > +static int tee_ioctl_invoke(struct tee_context *ctx,
> > +			    struct tee_ioctl_buf_data __user *ubuf)
> > +{
> > +	int rc;
> > +	size_t n;
> > +	struct tee_ioctl_buf_data buf;
> > +	struct tee_ioctl_invoke_arg __user *uarg;
> > +	struct tee_ioctl_invoke_arg arg;
> > +	struct tee_ioctl_param __user *uparams = NULL;
> > +	struct tee_param *params = NULL;
> > +
> > +	if (!ctx->teedev->desc->ops->invoke_func)
> > +		return -EINVAL;
> > +
> > +	rc = copy_from_user(&buf, ubuf, sizeof(buf));
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (buf.buf_len > TEE_MAX_ARG_SIZE ||
> > +	    buf.buf_len < sizeof(struct tee_ioctl_invoke_arg))
> > +		return -EINVAL;
> > +
> > +	uarg = (struct tee_ioctl_invoke_arg __user *)(unsigned long)buf.buf_ptr;
> > +	if (copy_from_user(&arg, uarg, sizeof(arg)))
> > +		return -EFAULT;
> > +
> > +	if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len)
> > +		return -EINVAL;
> > +
> > +	if (arg.num_params) {
> > +		params = kcalloc(arg.num_params, sizeof(struct tee_param),
> > +				 GFP_KERNEL);
> > +		if (!params)
> > +			return -ENOMEM;
> > +		uparams = (struct tee_ioctl_param __user *)(uarg + 1);
> > +		rc = params_from_user(ctx, params, arg.num_params, uparams);
> > +		if (rc)
> > +			goto out;
> > +	}
> > +
> > +	rc = ctx->teedev->desc->ops->invoke_func(ctx, &arg, params);
> > +	if (rc)
> > +		goto out;
> 
> Hmm.. I wonder if the teedev drivers should get subsystem level lock
> protection for ops invocation or should they implement locking themselves?

At least for OP-TEE we need to be able to make several ops invocations
in parallel so it locking has to be dealt with in the ops invocations
themselves.

Thanks for taking the time to review this.

--
Regards,
Jens

[toc] | [prev] | [next] | [standalone]

#1416264 — Re: [PATCH v10 2/4] tee: generic TEE subsystem

FromJoe Perches <joe@perches.com>
Date2016-06-07 16:40 +0200
SubjectRe: [PATCH v10 2/4] tee: generic TEE subsystem
Message-ID<rHqkF-3QU-11@gated-at.bofh.it>
In reply to#1415986
On Tue, 2016-06-07 at 12:50 +0200, Jens Wiklander wrote:
> On Mon, Jun 06, 2016 at 04:44:42PM -0500, Nishanth Menon wrote:
> > 
> > On 06/01/2016 07:41 AM, Jens Wiklander wrote:
> > few minor comments below.
> > 
> > I see the patch generated (with --strict):
> > > 
> > > CHECK: Alignment should match open parenthesis
> > > #512: FILE: drivers/tee/tee.c:375:
> > > +static int tee_ioctl_close_session(struct tee_context *ctx,
> > > +              struct tee_ioctl_close_session_arg __user *uarg)
> > > CHECK: Alignment should match open parenthesis
> > > #1607: FILE: drivers/tee/tee_shm_pool.c:103:
> > > +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
> > > +                      struct tee_shm_pool_mem_info *priv_info,
> > > CHECK: Alignment should match open parenthesis
> The alternative is to format it as:
> struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev,
>                                                 struct tee_shm_pool_mem_info
>                                                         *priv_info,
>                                                 struct tee_shm_pool_mem_info
>                                                         *dmabuf_info)
> But that is a bit awkward. I'd like to keep it as it is if you don't mind.

another style uses a separate line for the return type

struct tee_shm_pool *
tee_shm_pool_alloc_res_mem(struct device *dev,
			   struct tee_shm_pool_mem_info *priv_info,
			   struct tee_shm_pool_mem_info *dmabuf_info)

[toc] | [prev] | [next] | [standalone]

#1415507 — Re: [PATCH v10 3/4] tee: add OP-TEE driver

FromNishanth Menon <nm@ti.com>
Date2016-06-07 00:00 +0200
SubjectRe: [PATCH v10 3/4] tee: add OP-TEE driver
Message-ID<rHaIV-25i-19@gated-at.bofh.it>
In reply to#1411177
On 06/01/2016 07:41 AM, Jens Wiklander wrote:
[...]
> diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> index 60d2dab..53f3c76 100644
> --- a/drivers/tee/Makefile
> +++ b/drivers/tee/Makefile
> @@ -1,3 +1,4 @@
>  obj-y += tee.o
>  obj-y += tee_shm.o
>  obj-y += tee_shm_pool.o
> +obj-$(CONFIG_OPTEE) += optee/
> diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> new file mode 100644
> index 0000000..a7a8b71
> --- /dev/null
> +++ b/drivers/tee/optee/Kconfig
> @@ -0,0 +1,8 @@
> +# OP-TEE Trusted Execution Environment Configuration
> +config OPTEE
> +	tristate "OP-TEE"
> +	default n
You should'nt need this.

> +	depends on HAVE_ARM_SMCCC

HAVE_ARM_SMCCC might depend on OPTEE secure support in place, right? I
wonder if setsup any constraints for having a single zImage for OPTEE
and non-OPTEE systems, I think not.. just wondering.

Further, at this patch, smatch[1] complains:
> +drivers/tee/optee/core.c:488 optee_probe() error: we previously assumed 'optee' could be null (see line 444) 

Checkpatch --strict complains:

> +CHECK: Alignment should match open parenthesis
> +#878: FILE: drivers/tee/optee/core.c:333:
> ++static struct tee_shm_pool *optee_config_shm_ioremap(struct device *dev,
> ++                      optee_invoke_fn *invoke_fn,


[1] git://repo.or.cz/smatch.git
-- 
Regards,
Nishanth Menon

[toc] | [prev] | [next] | [standalone]

#1416075 — Re: [PATCH v10 3/4] tee: add OP-TEE driver

FromJens Wiklander <jens.wiklander@linaro.org>
Date2016-06-07 14:00 +0200
SubjectRe: [PATCH v10 3/4] tee: add OP-TEE driver
Message-ID<rHnPQ-2e7-11@gated-at.bofh.it>
In reply to#1415507
On Mon, Jun 06, 2016 at 04:49:57PM -0500, Nishanth Menon wrote:
> On 06/01/2016 07:41 AM, Jens Wiklander wrote:
> [...]
> > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile
> > index 60d2dab..53f3c76 100644
> > --- a/drivers/tee/Makefile
> > +++ b/drivers/tee/Makefile
> > @@ -1,3 +1,4 @@
> >  obj-y += tee.o
> >  obj-y += tee_shm.o
> >  obj-y += tee_shm_pool.o
> > +obj-$(CONFIG_OPTEE) += optee/
> > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > new file mode 100644
> > index 0000000..a7a8b71
> > --- /dev/null
> > +++ b/drivers/tee/optee/Kconfig
> > @@ -0,0 +1,8 @@
> > +# OP-TEE Trusted Execution Environment Configuration
> > +config OPTEE
> > +	tristate "OP-TEE"
> > +	default n
> You should'nt need this.

I'll fix.

> 
> > +	depends on HAVE_ARM_SMCCC
> 
> HAVE_ARM_SMCCC might depend on OPTEE secure support in place, right? I
> wonder if setsup any constraints for having a single zImage for OPTEE
> and non-OPTEE systems, I think not.. just wondering.

No, HAVE_ARM_SMCCC indicates just the presence of the arm_smccc_smc()
and arm_smccc_hvc() assembly functions. They are used by PSCI
independent of any TEE driver also.

> 
> Further, at this patch, smatch[1] complains:
> > +drivers/tee/optee/core.c:488 optee_probe() error: we previously assumed 'optee' could be null (see line 444) 

Sorry, I'll fix.

> 
> Checkpatch --strict complains:
> 
> > +CHECK: Alignment should match open parenthesis
> > +#878: FILE: drivers/tee/optee/core.c:333:
> > ++static struct tee_shm_pool *optee_config_shm_ioremap(struct device *dev,
> > ++                      optee_invoke_fn *invoke_fn,

Fixing this warning would make it less readable in my opinion, I'd
rather keep it as it is if you don't mind.

--
Thanks,
Jens

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web