From mboxrd@z Thu Jan 1 00:00:00 1970 From: jens.wiklander@linaro.org (Jens Wiklander) Date: Thu, 19 Jan 2017 17:45:43 +0100 Subject: [PATCH v13 2/5] tee: generic TEE subsystem In-Reply-To: <5825882.vZRDMrBMkW@wuerfel> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <1479480700-554-3-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> Message-ID: <20170119164541.GA22094@jax> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 and > > Emmanuel MICHEL > > * "Generic TrustZone Driver" by Javier Gonz?lez > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jens Wiklander Subject: Re: [PATCH v13 2/5] tee: generic TEE subsystem Date: Thu, 19 Jan 2017 17:45:43 +0100 Message-ID: <20170119164541.GA22094@jax> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <1479480700-554-3-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <5825882.vZRDMrBMkW@wuerfel> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Arnd Bergmann Cc: valentin.manea@huawei.com, devicetree@vger.kernel.org, javier@javigon.com, emmanuel.michel@st.com, Greg Kroah-Hartman , Mark Rutland , Will Deacon , linux-kernel@vger.kernel.org, Wei Xu , Nishanth Menon , Jason Gunthorpe , Rob Herring , broonie@kernel.org, Al Viro , "Andrew F. Davis" , Olof Johansson , Andrew Morton , jean-michel.delorme@st.com, Michal Simek , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgQXJuZCwKClRoaXMgaXMgdGhlIG9sZCB2MTMgcGF0Y2ggc2V0IHlvdSdyZSBjb21tZW50aW5n LCBidXQgaXQgZG9lc24ndCBtYXR0ZXIKaXQncyBlc3NlbnRpYWxseSBpZGVudGljYWwgdG8gdjE0 IGluIHRoaXMgcGF0Y2guCgpPbiBXZWQsIEphbiAxOCwgMjAxNyBhdCAwOToxOToyNVBNICswMTAw LCBBcm5kIEJlcmdtYW5uIHdyb3RlOgo+IE9uIEZyaWRheSwgTm92ZW1iZXIgMTgsIDIwMTYgMzo1 MTozNyBQTSBDRVQgSmVucyBXaWtsYW5kZXIgd3JvdGU6Cj4gPiBJbml0aWFsIHBhdGNoIGZvciBn ZW5lcmljIFRFRSBzdWJzeXN0ZW0uCj4gPiBUaGlzIHN1YnN5c3RlbSBwcm92aWRlczoKPiA+ICog UmVnaXN0cmF0aW9uL3VuLXJlZ2lzdHJhdGlvbiBvZiBURUUgZHJpdmVycy4KPiA+ICogU2hhcmVk IG1lbW9yeSBiZXR3ZWVuIG5vcm1hbCB3b3JsZCBhbmQgc2VjdXJlIHdvcmxkLgo+ID4gKiBJb2N0 bCBpbnRlcmZhY2UgZm9yIGludGVyYWN0aW9uIHdpdGggdXNlciBzcGFjZS4KPiA+ICogU3lzZnMg aW1wbGVtZW50YXRpb25faWQgb2YgVEVFIGRyaXZlcgo+ID4gCj4gPiBBIFRFRSAoVHJ1c3RlZCBF eGVjdXRpb24gRW52aXJvbm1lbnQpIGRyaXZlciBpcyBhIGRyaXZlciB0aGF0IGludGVyZmFjZXMK PiA+IHdpdGggYSB0cnVzdGVkIE9TIHJ1bm5pbmcgaW4gc29tZSBzZWN1cmUgZW52aXJvbm1lbnQs IGZvciBleGFtcGxlLAo+ID4gVHJ1c3Rab25lIG9uIEFSTSBjcHVzLCBvciBhIHNlcGFyYXRlIHNl Y3VyZSBjby1wcm9jZXNzb3IgZXRjLgo+ID4gCj4gPiBUaGUgVEVFIHN1YnN5c3RlbSBjYW4gc2Vy dmUgYSBURUUgZHJpdmVyIGZvciBhIEdsb2JhbCBQbGF0Zm9ybSBjb21wbGlhbnQKPiA+IFRFRSwg YnV0IGl0J3Mgbm90IGxpbWl0ZWQgdG8gb25seSBHbG9iYWwgUGxhdGZvcm0gVEVFcy4KPiA+IAo+ ID4gVGhpcyBwYXRjaCBidWlsZHMgb24gb3RoZXIgc2ltaWxhciBpbXBsZW1lbnRhdGlvbnMgdHJ5 aW5nIHRvIHNvbHZlCj4gPiB0aGUgc2FtZSBwcm9ibGVtOgo+ID4gKiAib3B0ZWVfbGludXhkcml2 ZXIiIGJ5IGFtb25nIG90aGVycwo+ID4gICBKZWFuLW1pY2hlbCBERUxPUk1FPGplYW4tbWljaGVs LmRlbG9ybWVAc3QuY29tPiBhbmQKPiA+ICAgRW1tYW51ZWwgTUlDSEVMIDxlbW1hbnVlbC5taWNo ZWxAc3QuY29tPgo+ID4gKiAiR2VuZXJpYyBUcnVzdFpvbmUgRHJpdmVyIiBieSBKYXZpZXIgR29u esOhbGV6IDxqYXZpZXJAamF2aWdvbi5jb20+Cj4gCj4gQ2FuIHlvdSBnaXZlIGFuIGV4YW1wbGUg Zm9yIGEgc3lzdGVtIHRoYXQgd291bGQgY29udGFpbiBtb3JlIHRoYW4gb25lCj4gVEVFPyBJIHNl ZSB0aGF0IHlvdSBzdXBwb3J0IGR5bmFtaWMgcmVnaXN0cmF0aW9uLCBhbmQgaXQncyBjbGVhciB0 aGF0Cj4gdGhlcmUgY2FuIGJlIG1vcmUgdGhhbiBvbmUgdHlwZSBvZiBURUUsIGJ1dCB3aHkgd291 bGQgb25lIGhhdmUgbW9yZQo+IHRoYW4gb25lIGF0IGEgdGltZSwgYW5kIHdoeSBub3QgbW9yZSB0 aGFuIDMyPwoKSSBrbm93IHRoYXQgU1QgaGFzIHN5c3RlbXMgd2hlcmUgdGhlcmUncyBvbmUgVEVF IGluIFRydXN0Wm9uZSBhbmQKYW5vdGhlciBURUUgb24gYSBzZXBhcmF0ZSBzZWN1cmUgY28tcHJv Y2Vzc29yLiBJZiB5b3UgaGF2ZSBzZXZlcmFsIFRFRXMKaXQncyBwcm9iYWJseSBiZWNhdXNlIHRo ZXkgaGF2ZSBkaWZmZXJlbnQgY2FwYWJpbGl0aWVzIChwZXJmb3JtYW5jZQp2ZXJzdXMgbGV2ZWwg b2Ygc2VjdXJpdHkpLiBKdXN0IGdvaW5nIGJleW9uZCB0d28gb3IgdGhyZWUgZGlmZmVyZW50Cmxl dmVscyBvZiBzZWN1cml0eSB3aXRoIGRpZmZlcmVudCBURUVzIHNvdW5kcyBhIGJpdCBleHRyZW1l LCBzbyBhCm1heGltdW0gb2YgMzIgb3IgMTYgc2hvdWxkIGJlIGZhaXJseSBzYWZlLiBJZiBpdCB0 dXJucyBvdXQgSSdtIHdyb25nIGluCnRoaXMgYXNzdW1wdGlvbiBpdCdzIG5vdCB0aGF0IGhhcmQg dG8gY29ycmVjdCBpdC4KCj4gCj4gPiArc3RhdGljIGludCB0ZWVfaW9jdGxfaW52b2tlKHN0cnVj dCB0ZWVfY29udGV4dCAqY3R4LAo+ID4gKwkJCSAgICBzdHJ1Y3QgdGVlX2lvY3RsX2J1Zl9kYXRh IF9fdXNlciAqdWJ1ZikKPiA+ICt7Cj4gPiArCWludCByYzsKPiA+ICsJc2l6ZV90IG47Cj4gPiAr CXN0cnVjdCB0ZWVfaW9jdGxfYnVmX2RhdGEgYnVmOwo+ID4gKwlzdHJ1Y3QgdGVlX2lvY3RsX2lu dm9rZV9hcmcgX191c2VyICp1YXJnOwo+ID4gKwlzdHJ1Y3QgdGVlX2lvY3RsX2ludm9rZV9hcmcg YXJnOwo+ID4gKwlzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIF9fdXNlciAqdXBhcmFtcyA9IE5VTEw7 Cj4gPiArCXN0cnVjdCB0ZWVfcGFyYW0gKnBhcmFtcyA9IE5VTEw7Cj4gPiArCj4gPiArCWlmICgh Y3R4LT50ZWVkZXYtPmRlc2MtPm9wcy0+aW52b2tlX2Z1bmMpCj4gPiArCQlyZXR1cm4gLUVJTlZB TDsKPiA+ICsKPiA+ICsJaWYgKGNvcHlfZnJvbV91c2VyKCZidWYsIHVidWYsIHNpemVvZihidWYp KSkKPiA+ICsJCXJldHVybiAtRUZBVUxUOwo+ID4gKwo+ID4gKwlpZiAoYnVmLmJ1Zl9sZW4gPiBU RUVfTUFYX0FSR19TSVpFIHx8Cj4gPiArCSAgICBidWYuYnVmX2xlbiA8IHNpemVvZihzdHJ1Y3Qg dGVlX2lvY3RsX2ludm9rZV9hcmcpKQo+ID4gKwkJcmV0dXJuIC1FSU5WQUw7Cj4gPiArCj4gPiAr CXVhcmcgPSAoc3RydWN0IHRlZV9pb2N0bF9pbnZva2VfYXJnIF9fdXNlciAqKSh1bnNpZ25lZCBs b25nKWJ1Zi5idWZfcHRyOwo+IAo+IHU2NF90b191c2VyX3B0cigpCgpUaGFua3MsIHRoYXQncyBj b252ZW5pZW50LgoKPiAKPiA+ICsJaWYgKGNvcHlfZnJvbV91c2VyKCZhcmcsIHVhcmcsIHNpemVv ZihhcmcpKSkKPiA+ICsJCXJldHVybiAtRUZBVUxUOwo+ID4gKwo+ID4gKwlpZiAoc2l6ZW9mKGFy ZykgKyBURUVfSU9DVExfUEFSQU1fU0laRShhcmcubnVtX3BhcmFtcykgIT0gYnVmLmJ1Zl9sZW4p Cj4gPiArCQlyZXR1cm4gLUVJTlZBTDsKPiA+ICsKPiA+ICsJaWYgKGFyZy5udW1fcGFyYW1zKSB7 Cj4gPiArCQlwYXJhbXMgPSBrY2FsbG9jKGFyZy5udW1fcGFyYW1zLCBzaXplb2Yoc3RydWN0IHRl ZV9wYXJhbSksCj4gPiArCQkJCSBHRlBfS0VSTkVMKTsKPiA+ICsJCWlmICghcGFyYW1zKQo+ID4g KwkJCXJldHVybiAtRU5PTUVNOwo+IAo+IEl0IHdvdWxkIGJlIGdvb2QgdG8gaGF2ZSBhbiB1cHBl ciBib3VuZCBvbiB0aGUgbnVtYmVyIG9mIHBhcmFtZXRlcnMKPiB0byBsaW1pdCB0aGUgc2l6ZSBv ZiB0aGUgbWVtb3J5IGFsbG9jYXRpb24gaGVyZS4KClRoaXMgaXMgYWxyZWFkeSBsaW1pdGVkIGR1 ZSB0bzoKClRoZSB0ZXN0IHdpdGg6IGJ1Zi5idWZfbGVuID4gVEVFX01BWF9BUkdfU0laRQoKQW5k IHRoZW4gYW5vdGhlciB0ZXN0IHRoYXQgdGhlIG51bWJlciBvZiBwYXJhbWV0ZXJzIG1hdGNoZXMg dGhlIGJ1ZmZlciBzaXplCndpdGg6IHNpemVvZihhcmcpICsgVEVFX0lPQ1RMX1BBUkFNX1NJWkUo YXJnLm51bV9wYXJhbXMpICE9IGJ1Zi5idWZfbGVuCgo+IAo+ID4gK2ludCB0ZWVfZGV2aWNlX3Jl Z2lzdGVyKHN0cnVjdCB0ZWVfZGV2aWNlICp0ZWVkZXYpCj4gPiArewo+ID4gKwlpbnQgcmM7Cj4g PiArCj4gPiArCS8qCj4gPiArCSAqIElmIHRoZSB0ZWVkZXYgYWxyZWFkeSBpcyByZWdpc3RlcmVk LCBkb24ndCBkbyBpdCBhZ2Fpbi4gSXQncwo+ID4gKwkgKiBvYnZpb3VzbHkgYW4gZXJyb3IgdG8g dHJ5IHRvIHJlZ2lzdGVyIHR3aWNlLCBidXQgaWYgd2UgcmV0dXJuCj4gPiArCSAqIGFuIGVycm9y IHdlJ2xsIGZvcmNlIHRoZSBkcml2ZXIgdG8gcmVtb3ZlIHRoZSB0ZWVkZXYuCj4gPiArCSAqLwo+ ID4gKwlpZiAodGVlZGV2LT5mbGFncyAmIFRFRV9ERVZJQ0VfRkxBR19SRUdJU1RFUkVEKSB7Cj4g PiArCQlkZXZfZXJyKCZ0ZWVkZXYtPmRldiwgImF0dGVtcHQgdG8gcmVnaXN0ZXIgdHdpY2VcbiIp Owo+ID4gKwkJcmV0dXJuIDA7Cj4gPiArCX0KPiAKPiBJIGRvbid0IHVuZGVyc3RhbmQgd2hhdCB5 b3UgYXJlIHByb3RlY3RpbmcgYWdhaW5zdCBoZXJlLgo+IEhvdyB3b3VsZCB3ZSBnZXQgdG8gdGhp cyBmdW5jdGlvbiB0d2ljZSBmb3IgdGhlIHNhbWUgZGV2aWNlPwo+IAo+IENvdWxkIHlvdSBjaGFu Z2UgdGhlIGNhbGxlciBzbyBpdCBkb2Vzbid0IGhhcHBlbj8KClllcyB0aGUgY2FsbGVyIGNhbiBi ZSBjaGFuZ2VkLCBJJ2xsIHJldHVybiBhbiBlcnJvciBpbnN0ZWFkIChhbmQgcmVtb3ZlCnRoZSBj b21tZW50KS4KCj4gCj4gPiArLyoqCj4gPiArICogc3RydWN0IHRlZV9pb2N0bF9wYXJhbSAtIHBh cmFtZXRlcgo+ID4gKyAqIEBhdHRyOiBhdHRyaWJ1dGVzCj4gPiArICogQG1lbXJlZjogYSBtZW1v cnkgcmVmZXJlbmNlCj4gPiArICogQHZhbHVlOiBhIHZhbHVlCj4gPiArICoKPiA+ICsgKiBAYXR0 ciAmIFRFRV9QQVJBTV9BVFRSX1RZUEVfTUFTSyBpbmRpY2F0ZXMgaWYgbWVtcmVmIG9yIHZhbHVl IGlzIHVzZWQgaW4KPiA+ICsgKiB0aGUgdW5pb24uIFRFRV9QQVJBTV9BVFRSX1RZUEVfVkFMVUVf KiBpbmRpY2F0ZXMgdmFsdWUgYW5kCj4gPiArICogVEVFX1BBUkFNX0FUVFJfVFlQRV9NRU1SRUZf KiBpbmRpY2F0ZXMgbWVtcmVmLiBURUVfUEFSQU1fQVRUUl9UWVBFX05PTkUKPiA+ICsgKiBpbmRp Y2F0ZXMgdGhhdCBub25lIG9mIHRoZSBtZW1iZXJzIGFyZSB1c2VkLgo+ID4gKyAqLwo+ID4gK3N0 cnVjdCB0ZWVfaW9jdGxfcGFyYW0gewo+ID4gKwlfX3U2NCBhdHRyOwo+ID4gKwl1bmlvbiB7Cj4g PiArCQlzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtX21lbXJlZiBtZW1yZWY7Cj4gPiArCQlzdHJ1Y3Qg dGVlX2lvY3RsX3BhcmFtX3ZhbHVlIHZhbHVlOwo+ID4gKwl9IHU7Cj4gPiArfTsKPiA+ICsKPiA+ ICsjZGVmaW5lIFRFRV9JT0NUTF9VVUlEX0xFTgkJMTYKPiA+ICsKPiAKPiBIYXZpbmcgYSB1bmlv biBpbiBhbiBpb2N0bCBhcmd1bWVudCBzZWVtcyBvZGQuIEhhdmUgeW91IGNvbnNpZGVyZWQKPiB1 c2luZyB0d28gZGlmZmVyZW50IGlvY3RsIGNvbW1hbmQgbnVtYmVycyBkZXBlbmRpbmcgb24gdGhl IHR5cGU/CgpzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIGlzIHVzZWQgYXMgYW4gYXJyYXkgYW5kIHNv bWUgcGFyYW1ldGVycyBjYW4gYmUKbWVtcmVmcyB3aGlsZSBvdGhlciBhcmUgdmFsdWVzLgoKPiAK PiA+ICsvKioKPiA+ICsgKiBzdHJ1Y3QgdGVlX2lvY2xfc3VwcF9zZW5kX2FyZyAtIFNlbmQgYSBy ZXNwb25zZSB0byBhIHJlY2VpdmVkIHJlcXVlc3QKPiA+ICsgKiBAcmV0Oglbb3V0XSByZXR1cm4g dmFsdWUKPiA+ICsgKiBAbnVtX3BhcmFtcwlbaW5dIG51bWJlciBvZiBwYXJhbWV0ZXJzIGZvbGxv d2luZyB0aGlzIHN0cnVjdAo+ID4gKyAqLwo+ID4gK3N0cnVjdCB0ZWVfaW9jbF9zdXBwX3NlbmRf YXJnIHsKPiA+ICsJX191MzIgcmV0Owo+ID4gKwlfX3UzMiBudW1fcGFyYW1zOwo+ID4gKwkvKgo+ ID4gKwkgKiB0aGlzIHN0cnVjdCBpcyA4IGJ5dGUgYWxpZ25lZCBzaW5jZSB0aGUgJ3N0cnVjdCB0 ZWVfaW9jdGxfcGFyYW0nCj4gPiArCSAqIHdoaWNoIGZvbGxvd3MgcmVxdWlyZXMgOCBieXRlIGFs aWdubWVudC4KPiA+ICsJICoKPiA+ICsJICogQ29tbWVudGVkIG91dCBlbGVtZW50IHVzZWQgdG8g dmlzdWFsaXplIHRoZSBsYXlvdXQgZHluYW1pYyBwYXJ0Cj4gPiArCSAqIG9mIHRoZSBzdHJ1Y3Qu IFRoaXMgZmllbGQgaXMgbm90IGF2YWlsYWJsZSBhdCBhbGwgaWYKPiA+ICsJICogbnVtX3BhcmFt cyA9PSAwLgo+ID4gKwkgKgo+ID4gKwkgKiBzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIHBhcmFtc1tu dW1fcGFyYW1zXTsKPiA+ICsJICovCj4gPiArfSBfX2FsaWduZWQoOCk7Cj4gCj4gSSdkIG1ha2Ug dGhhdCAKPiAKPiAJc3RydWN0IHRlZV9pb2N0bF9wYXJhbSBwYXJhbXNbMF07Cj4gCj4gYXMgd2Vs IGhlcmUsIGFzIEkgYWxzbyBjb21tZW50ZWQgaW4gcGF0Y2ggMyB0aGF0IGhhcyBhIHNpbWlsYXIg c3RydWN0dXJlLgoKSSdtIGNvbmNlcm5lZCB0aGF0IHRoaXMgbWF5IGNhdXNlIHdhcm5pbmdzIHdo ZW4gY29tcGlsaW5nIGZvciB1c2VyIHNwYWNlCmRlcGVuZGluZyBvbiBjb21waWxlciBhbmQgb3B0 aW9ucy4gQW0gSSB0b28gY2F1dGlvdXMgaGVyZT8KIApUaGFua3MsCkplbnMKCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFp bGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlz dHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753579AbdASQp4 (ORCPT ); Thu, 19 Jan 2017 11:45:56 -0500 Received: from mail-lf0-f41.google.com ([209.85.215.41]:35012 "EHLO mail-lf0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753431AbdASQps (ORCPT ); Thu, 19 Jan 2017 11:45:48 -0500 Date: Thu, 19 Jan 2017 17:45:43 +0100 From: Jens Wiklander To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, Greg Kroah-Hartman , Olof Johansson , Andrew Morton , Wei Xu , valentin.manea@huawei.com, devicetree@vger.kernel.org, javier@javigon.com, emmanuel.michel@st.com, Nishanth Menon , broonie@kernel.org, Will Deacon , linux-kernel@vger.kernel.org, jean-michel.delorme@st.com, Jason Gunthorpe , Rob Herring , Al Viro , Mark Rutland , "Andrew F. Davis" , Michal Simek Subject: Re: [PATCH v13 2/5] tee: generic TEE subsystem Message-ID: <20170119164541.GA22094@jax> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <1479480700-554-3-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5825882.vZRDMrBMkW@wuerfel> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 and > > Emmanuel MICHEL > > * "Generic TrustZone Driver" by Javier González > > 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