From: Jens Wiklander <jens.wiklander@linaro.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel@lists.infradead.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Olof Johansson <olof@lixom.net>,
Andrew Morton <akpm@linux-foundation.org>,
Wei Xu <xuwei5@hisilicon.com>,
valentin.manea@huawei.com, devicetree@vger.kernel.org,
javier@javigon.com, emmanuel.michel@st.com,
Nishanth Menon <nm@ti.com>,
broonie@kernel.org, Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, jean-michel.delorme@st.com,
Jason Gunthorpe <jgunthorpe@obsidianresearch.com>,
Rob Herring <robh+dt@kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>,
Mark Rutland <mark.rutland@arm.com>,
"Andrew F. Davis" <afd@ti.com>,
Michal Simek <michal.simek@xilinx.com>
Subject: Re: [PATCH v13 2/5] tee: generic TEE subsystem
Date: Thu, 19 Jan 2017 17:45:43 +0100 [thread overview]
Message-ID: <20170119164541.GA22094@jax> (raw)
In-Reply-To: <5825882.vZRDMrBMkW@wuerfel>
Hi Arnd,
This is the old v13 patch set you're commenting, but it doesn't matter
it's essentially identical to v14 in this patch.
On Wed, Jan 18, 2017 at 09:19:25PM +0100, Arnd Bergmann wrote:
> On Friday, November 18, 2016 3:51:37 PM CET Jens Wiklander wrote:
> > Initial patch for generic TEE subsystem.
> > This subsystem provides:
> > * Registration/un-registration of TEE drivers.
> > * Shared memory between normal world and secure world.
> > * Ioctl interface for interaction with user space.
> > * Sysfs implementation_id of TEE driver
> >
> > A TEE (Trusted Execution Environment) driver is a driver that interfaces
> > with a trusted OS running in some secure environment, for example,
> > TrustZone on ARM cpus, or a separate secure co-processor etc.
> >
> > The TEE subsystem can serve a TEE driver for a Global Platform compliant
> > TEE, but it's not limited to only Global Platform TEEs.
> >
> > This patch builds on other similar implementations trying to solve
> > the same problem:
> > * "optee_linuxdriver" by among others
> > Jean-michel DELORME<jean-michel.delorme@st.com> and
> > Emmanuel MICHEL <emmanuel.michel@st.com>
> > * "Generic TrustZone Driver" by Javier González <javier@javigon.com>
>
> Can you give an example for a system that would contain more than one
> TEE? I see that you support dynamic registration, and it's clear that
> there can be more than one type of TEE, but why would one have more
> than one at a time, and why not more than 32?
I know that ST has systems where there's one TEE in TrustZone and
another TEE on a separate secure co-processor. If you have several TEEs
it's probably because they have different capabilities (performance
versus level of security). Just going beyond two or three different
levels of security with different TEEs sounds a bit extreme, so a
maximum of 32 or 16 should be fairly safe. If it turns out I'm wrong in
this assumption it's not that hard to correct it.
>
> > +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;
> > +
> > + if (copy_from_user(&buf, ubuf, sizeof(buf)))
> > + return -EFAULT;
> > +
> > + 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;
>
> u64_to_user_ptr()
Thanks, that's convenient.
>
> > + 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;
>
> It would be good to have an upper bound on the number of parameters
> to limit the size of the memory allocation here.
This is already limited due to:
The test with: buf.buf_len > TEE_MAX_ARG_SIZE
And then another test that the number of parameters matches the buffer size
with: sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len
>
> > +int tee_device_register(struct tee_device *teedev)
> > +{
> > + int rc;
> > +
> > + /*
> > + * If the teedev already is registered, don't do it again. It's
> > + * obviously an error to try to register twice, but if we return
> > + * an error we'll force the driver to remove the teedev.
> > + */
> > + if (teedev->flags & TEE_DEVICE_FLAG_REGISTERED) {
> > + dev_err(&teedev->dev, "attempt to register twice\n");
> > + return 0;
> > + }
>
> I don't understand what you are protecting against here.
> How would we get to this function twice for the same device?
>
> Could you change the caller so it doesn't happen?
Yes the caller can be changed, I'll return an error instead (and remove
the comment).
>
> > +/**
> > + * struct tee_ioctl_param - parameter
> > + * @attr: attributes
> > + * @memref: a memory reference
> > + * @value: a value
> > + *
> > + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in
> > + * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and
> > + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE
> > + * indicates that none of the members are used.
> > + */
> > +struct tee_ioctl_param {
> > + __u64 attr;
> > + union {
> > + struct tee_ioctl_param_memref memref;
> > + struct tee_ioctl_param_value value;
> > + } u;
> > +};
> > +
> > +#define TEE_IOCTL_UUID_LEN 16
> > +
>
> Having a union in an ioctl argument seems odd. Have you considered
> using two different ioctl command numbers depending on the type?
struct tee_ioctl_param is used as an array and some parameters can be
memrefs while other are values.
>
> > +/**
> > + * struct tee_iocl_supp_send_arg - Send a response to a received request
> > + * @ret: [out] return value
> > + * @num_params [in] number of parameters following this struct
> > + */
> > +struct tee_iocl_supp_send_arg {
> > + __u32 ret;
> > + __u32 num_params;
> > + /*
> > + * this struct is 8 byte aligned since the 'struct tee_ioctl_param'
> > + * which follows requires 8 byte alignment.
> > + *
> > + * Commented out element used to visualize the layout dynamic part
> > + * of the struct. This field is not available at all if
> > + * num_params == 0.
> > + *
> > + * struct tee_ioctl_param params[num_params];
> > + */
> > +} __aligned(8);
>
> I'd make that
>
> struct tee_ioctl_param params[0];
>
> as wel here, as I also commented in patch 3 that has a similar structure.
I'm concerned that this may cause warnings when compiling for user space
depending on compiler and options. Am I too cautious here?
Thanks,
Jens
next prev parent reply other threads:[~2017-01-19 16:45 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-18 14:51 [PATCH v13 0/5] generic TEE subsystem Jens Wiklander
2016-11-18 14:51 ` [PATCH v13 1/5] dt/bindings: add bindings for optee Jens Wiklander
2016-11-18 14:51 ` [PATCH v13 2/5] tee: generic TEE subsystem Jens Wiklander
2017-01-18 20:19 ` Arnd Bergmann
2017-01-19 16:45 ` Jens Wiklander [this message]
2017-01-20 16:47 ` Arnd Bergmann
2017-01-23 8:20 ` Jens Wiklander
2016-11-18 14:51 ` [PATCH v13 3/5] tee: add OP-TEE driver Jens Wiklander
2016-11-18 14:51 ` [PATCH v13 4/5] Documentation: tee subsystem and op-tee driver Jens Wiklander
2016-11-18 14:51 ` [PATCH v13 5/5] arm64: dt: hikey: Add optee node Jens Wiklander
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170119164541.GA22094@jax \
--to=jens.wiklander@linaro.org \
--cc=afd@ti.com \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=emmanuel.michel@st.com \
--cc=gregkh@linuxfoundation.org \
--cc=javier@javigon.com \
--cc=jean-michel.delorme@st.com \
--cc=jgunthorpe@obsidianresearch.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=michal.simek@xilinx.com \
--cc=nm@ti.com \
--cc=olof@lixom.net \
--cc=robh+dt@kernel.org \
--cc=valentin.manea@huawei.com \
--cc=viro@zeniv.linux.org.uk \
--cc=will.deacon@arm.com \
--cc=xuwei5@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).