From mboxrd@z Thu Jan 1 00:00:00 1970 From: jens.wiklander@linaro.org (Jens Wiklander) Date: Mon, 23 Jan 2017 09:20:02 +0100 Subject: [PATCH v13 2/5] tee: generic TEE subsystem In-Reply-To: <8436100.O3a8W3D0ph@wuerfel> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> <20170119164541.GA22094@jax> <8436100.O3a8W3D0ph@wuerfel> Message-ID: <20170123082001.GA1910@jax> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Jan 20, 2017 at 05:47:40PM +0100, Arnd Bergmann wrote: > On Thursday, January 19, 2017 5:45:43 PM CET Jens Wiklander wrote: > > > > 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. > > Ok > > > > > > > > + 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 > > Ok, makes sense. > > > > > > > > +/** > > > > + * 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. > > Got it. I still think it's a bit awkward on the user ABI side. > I also see that (unlike the in-kernel interface) tee_ioctl_param_memref > and tee_ioctl_param_value are both defined in terms of three __u64 > members. Actually the last member of struct tee_ioctl_param_memref is an __s64, but that doesn't matter much one just have to be a little careful. > > How about simply using one format here and making this > > struct tee_ioctl_param { > __u64 attr; > __u64 a; > __u64 b; > __u64 c; > }; > > Given that you need a wrapper to set the pointer in memref anyway? > > Having an ioctl with a variable number of variable type arguments > is really a weakness of the ABI, but I don't see a good way around > it either, the above would just make it slightly more direct. OK, I'll change. > > > > > +/** > > > > + * 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? > > See https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html > > I actually misremembered it and the syntax I listed is GCC specific, > but C99 allows "flexible arrays". I think there is no problem relying > on C99 here, we already rely on C99 features elsewhere in headers. OK, I'll change. 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: Mon, 23 Jan 2017 09:20:02 +0100 Message-ID: <20170123082001.GA1910@jax> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> <20170119164541.GA22094@jax> <8436100.O3a8W3D0ph@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <8436100.O3a8W3D0ph@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 T24gRnJpLCBKYW4gMjAsIDIwMTcgYXQgMDU6NDc6NDBQTSArMDEwMCwgQXJuZCBCZXJnbWFubiB3 cm90ZToKPiBPbiBUaHVyc2RheSwgSmFudWFyeSAxOSwgMjAxNyA1OjQ1OjQzIFBNIENFVCBKZW5z IFdpa2xhbmRlciB3cm90ZToKPiA+IAo+ID4gT24gV2VkLCBKYW4gMTgsIDIwMTcgYXQgMDk6MTk6 MjVQTSArMDEwMCwgQXJuZCBCZXJnbWFubiB3cm90ZToKPiA+ID4gT24gRnJpZGF5LCBOb3ZlbWJl ciAxOCwgMjAxNiAzOjUxOjM3IFBNIENFVCBKZW5zIFdpa2xhbmRlciB3cm90ZToKPiA+ID4gPiBJ bml0aWFsIHBhdGNoIGZvciBnZW5lcmljIFRFRSBzdWJzeXN0ZW0uCj4gPiA+ID4gVGhpcyBzdWJz eXN0ZW0gcHJvdmlkZXM6Cj4gPiA+ID4gKiBSZWdpc3RyYXRpb24vdW4tcmVnaXN0cmF0aW9uIG9m IFRFRSBkcml2ZXJzLgo+ID4gPiA+ICogU2hhcmVkIG1lbW9yeSBiZXR3ZWVuIG5vcm1hbCB3b3Js ZCBhbmQgc2VjdXJlIHdvcmxkLgo+ID4gPiA+ICogSW9jdGwgaW50ZXJmYWNlIGZvciBpbnRlcmFj dGlvbiB3aXRoIHVzZXIgc3BhY2UuCj4gPiA+ID4gKiBTeXNmcyBpbXBsZW1lbnRhdGlvbl9pZCBv ZiBURUUgZHJpdmVyCj4gPiA+ID4gCj4gPiA+ID4gQSBURUUgKFRydXN0ZWQgRXhlY3V0aW9uIEVu dmlyb25tZW50KSBkcml2ZXIgaXMgYSBkcml2ZXIgdGhhdCBpbnRlcmZhY2VzCj4gPiA+ID4gd2l0 aCBhIHRydXN0ZWQgT1MgcnVubmluZyBpbiBzb21lIHNlY3VyZSBlbnZpcm9ubWVudCwgZm9yIGV4 YW1wbGUsCj4gPiA+ID4gVHJ1c3Rab25lIG9uIEFSTSBjcHVzLCBvciBhIHNlcGFyYXRlIHNlY3Vy ZSBjby1wcm9jZXNzb3IgZXRjLgo+ID4gPiA+IAo+ID4gPiA+IFRoZSBURUUgc3Vic3lzdGVtIGNh biBzZXJ2ZSBhIFRFRSBkcml2ZXIgZm9yIGEgR2xvYmFsIFBsYXRmb3JtIGNvbXBsaWFudAo+ID4g PiA+IFRFRSwgYnV0IGl0J3Mgbm90IGxpbWl0ZWQgdG8gb25seSBHbG9iYWwgUGxhdGZvcm0gVEVF cy4KPiA+ID4gPiAKPiA+ID4gPiBUaGlzIHBhdGNoIGJ1aWxkcyBvbiBvdGhlciBzaW1pbGFyIGlt cGxlbWVudGF0aW9ucyB0cnlpbmcgdG8gc29sdmUKPiA+ID4gPiB0aGUgc2FtZSBwcm9ibGVtOgo+ ID4gPiA+ICogIm9wdGVlX2xpbnV4ZHJpdmVyIiBieSBhbW9uZyBvdGhlcnMKPiA+ID4gPiAgIEpl YW4tbWljaGVsIERFTE9STUU8amVhbi1taWNoZWwuZGVsb3JtZUBzdC5jb20+IGFuZAo+ID4gPiA+ ICAgRW1tYW51ZWwgTUlDSEVMIDxlbW1hbnVlbC5taWNoZWxAc3QuY29tPgo+ID4gPiA+ICogIkdl bmVyaWMgVHJ1c3Rab25lIERyaXZlciIgYnkgSmF2aWVyIEdvbnrDoWxleiA8amF2aWVyQGphdmln b24uY29tPgo+ID4gPiAKPiA+ID4gQ2FuIHlvdSBnaXZlIGFuIGV4YW1wbGUgZm9yIGEgc3lzdGVt IHRoYXQgd291bGQgY29udGFpbiBtb3JlIHRoYW4gb25lCj4gPiA+IFRFRT8gSSBzZWUgdGhhdCB5 b3Ugc3VwcG9ydCBkeW5hbWljIHJlZ2lzdHJhdGlvbiwgYW5kIGl0J3MgY2xlYXIgdGhhdAo+ID4g PiB0aGVyZSBjYW4gYmUgbW9yZSB0aGFuIG9uZSB0eXBlIG9mIFRFRSwgYnV0IHdoeSB3b3VsZCBv bmUgaGF2ZSBtb3JlCj4gPiA+IHRoYW4gb25lIGF0IGEgdGltZSwgYW5kIHdoeSBub3QgbW9yZSB0 aGFuIDMyPwo+ID4gCj4gPiBJIGtub3cgdGhhdCBTVCBoYXMgc3lzdGVtcyB3aGVyZSB0aGVyZSdz IG9uZSBURUUgaW4gVHJ1c3Rab25lIGFuZAo+ID4gYW5vdGhlciBURUUgb24gYSBzZXBhcmF0ZSBz ZWN1cmUgY28tcHJvY2Vzc29yLiBJZiB5b3UgaGF2ZSBzZXZlcmFsIFRFRXMKPiA+IGl0J3MgcHJv YmFibHkgYmVjYXVzZSB0aGV5IGhhdmUgZGlmZmVyZW50IGNhcGFiaWxpdGllcyAocGVyZm9ybWFu Y2UKPiA+IHZlcnN1cyBsZXZlbCBvZiBzZWN1cml0eSkuIEp1c3QgZ29pbmcgYmV5b25kIHR3byBv ciB0aHJlZSBkaWZmZXJlbnQKPiA+IGxldmVscyBvZiBzZWN1cml0eSB3aXRoIGRpZmZlcmVudCBU RUVzIHNvdW5kcyBhIGJpdCBleHRyZW1lLCBzbyBhCj4gPiBtYXhpbXVtIG9mIDMyIG9yIDE2IHNo b3VsZCBiZSBmYWlybHkgc2FmZS4gSWYgaXQgdHVybnMgb3V0IEknbSB3cm9uZyBpbgo+ID4gdGhp cyBhc3N1bXB0aW9uIGl0J3Mgbm90IHRoYXQgaGFyZCB0byBjb3JyZWN0IGl0Lgo+IAo+IE9rCj4g Cj4gPiA+IAo+ID4gPiA+ICsJaWYgKGNvcHlfZnJvbV91c2VyKCZhcmcsIHVhcmcsIHNpemVvZihh cmcpKSkKPiA+ID4gPiArCQlyZXR1cm4gLUVGQVVMVDsKPiA+ID4gPiArCj4gPiA+ID4gKwlpZiAo c2l6ZW9mKGFyZykgKyBURUVfSU9DVExfUEFSQU1fU0laRShhcmcubnVtX3BhcmFtcykgIT0gYnVm LmJ1Zl9sZW4pCj4gPiA+ID4gKwkJcmV0dXJuIC1FSU5WQUw7Cj4gPiA+ID4gKwo+ID4gPiA+ICsJ aWYgKGFyZy5udW1fcGFyYW1zKSB7Cj4gPiA+ID4gKwkJcGFyYW1zID0ga2NhbGxvYyhhcmcubnVt X3BhcmFtcywgc2l6ZW9mKHN0cnVjdCB0ZWVfcGFyYW0pLAo+ID4gPiA+ICsJCQkJIEdGUF9LRVJO RUwpOwo+ID4gPiA+ICsJCWlmICghcGFyYW1zKQo+ID4gPiA+ICsJCQlyZXR1cm4gLUVOT01FTTsK PiA+ID4gCj4gPiA+IEl0IHdvdWxkIGJlIGdvb2QgdG8gaGF2ZSBhbiB1cHBlciBib3VuZCBvbiB0 aGUgbnVtYmVyIG9mIHBhcmFtZXRlcnMKPiA+ID4gdG8gbGltaXQgdGhlIHNpemUgb2YgdGhlIG1l bW9yeSBhbGxvY2F0aW9uIGhlcmUuCj4gPiAKPiA+IFRoaXMgaXMgYWxyZWFkeSBsaW1pdGVkIGR1 ZSB0bzoKPiA+IAo+ID4gVGhlIHRlc3Qgd2l0aDogYnVmLmJ1Zl9sZW4gPiBURUVfTUFYX0FSR19T SVpFCj4gPiAKPiA+IEFuZCB0aGVuIGFub3RoZXIgdGVzdCB0aGF0IHRoZSBudW1iZXIgb2YgcGFy YW1ldGVycyBtYXRjaGVzIHRoZSBidWZmZXIgc2l6ZQo+ID4gd2l0aDogc2l6ZW9mKGFyZykgKyBU RUVfSU9DVExfUEFSQU1fU0laRShhcmcubnVtX3BhcmFtcykgIT0gYnVmLmJ1Zl9sZW4KPiAKPiBP aywgbWFrZXMgc2Vuc2UuCj4gCj4gPiA+IAo+ID4gPiA+ICsvKioKPiA+ID4gPiArICogc3RydWN0 IHRlZV9pb2N0bF9wYXJhbSAtIHBhcmFtZXRlcgo+ID4gPiA+ICsgKiBAYXR0cjogYXR0cmlidXRl cwo+ID4gPiA+ICsgKiBAbWVtcmVmOiBhIG1lbW9yeSByZWZlcmVuY2UKPiA+ID4gPiArICogQHZh bHVlOiBhIHZhbHVlCj4gPiA+ID4gKyAqCj4gPiA+ID4gKyAqIEBhdHRyICYgVEVFX1BBUkFNX0FU VFJfVFlQRV9NQVNLIGluZGljYXRlcyBpZiBtZW1yZWYgb3IgdmFsdWUgaXMgdXNlZCBpbgo+ID4g PiA+ICsgKiB0aGUgdW5pb24uIFRFRV9QQVJBTV9BVFRSX1RZUEVfVkFMVUVfKiBpbmRpY2F0ZXMg dmFsdWUgYW5kCj4gPiA+ID4gKyAqIFRFRV9QQVJBTV9BVFRSX1RZUEVfTUVNUkVGXyogaW5kaWNh dGVzIG1lbXJlZi4gVEVFX1BBUkFNX0FUVFJfVFlQRV9OT05FCj4gPiA+ID4gKyAqIGluZGljYXRl cyB0aGF0IG5vbmUgb2YgdGhlIG1lbWJlcnMgYXJlIHVzZWQuCj4gPiA+ID4gKyAqLwo+ID4gPiA+ ICtzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIHsKPiA+ID4gPiArCV9fdTY0IGF0dHI7Cj4gPiA+ID4g Kwl1bmlvbiB7Cj4gPiA+ID4gKwkJc3RydWN0IHRlZV9pb2N0bF9wYXJhbV9tZW1yZWYgbWVtcmVm Owo+ID4gPiA+ICsJCXN0cnVjdCB0ZWVfaW9jdGxfcGFyYW1fdmFsdWUgdmFsdWU7Cj4gPiA+ID4g Kwl9IHU7Cj4gPiA+ID4gK307Cj4gPiA+ID4gKwo+ID4gPiA+ICsjZGVmaW5lIFRFRV9JT0NUTF9V VUlEX0xFTgkJMTYKPiA+ID4gPiArCj4gPiA+IAo+ID4gPiBIYXZpbmcgYSB1bmlvbiBpbiBhbiBp b2N0bCBhcmd1bWVudCBzZWVtcyBvZGQuIEhhdmUgeW91IGNvbnNpZGVyZWQKPiA+ID4gdXNpbmcg dHdvIGRpZmZlcmVudCBpb2N0bCBjb21tYW5kIG51bWJlcnMgZGVwZW5kaW5nIG9uIHRoZSB0eXBl Pwo+ID4gCj4gPiBzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIGlzIHVzZWQgYXMgYW4gYXJyYXkgYW5k IHNvbWUgcGFyYW1ldGVycyBjYW4gYmUKPiA+IG1lbXJlZnMgd2hpbGUgb3RoZXIgYXJlIHZhbHVl cy4KPiAKPiBHb3QgaXQuIEkgc3RpbGwgdGhpbmsgaXQncyBhIGJpdCBhd2t3YXJkIG9uIHRoZSB1 c2VyIEFCSSBzaWRlLgo+IEkgYWxzbyBzZWUgdGhhdCAodW5saWtlIHRoZSBpbi1rZXJuZWwgaW50 ZXJmYWNlKSB0ZWVfaW9jdGxfcGFyYW1fbWVtcmVmCj4gYW5kIHRlZV9pb2N0bF9wYXJhbV92YWx1 ZSBhcmUgYm90aCBkZWZpbmVkIGluIHRlcm1zIG9mIHRocmVlIF9fdTY0Cj4gbWVtYmVycy4KCkFj dHVhbGx5IHRoZSBsYXN0IG1lbWJlciBvZiBzdHJ1Y3QgdGVlX2lvY3RsX3BhcmFtX21lbXJlZiBp cyBhbiBfX3M2NCwKYnV0IHRoYXQgZG9lc24ndCBtYXR0ZXIgbXVjaCBvbmUganVzdCBoYXZlIHRv IGJlIGEgbGl0dGxlIGNhcmVmdWwuCgo+IAo+IEhvdyBhYm91dCBzaW1wbHkgdXNpbmcgb25lIGZv cm1hdCBoZXJlIGFuZCBtYWtpbmcgdGhpcwo+IAo+IHN0cnVjdCB0ZWVfaW9jdGxfcGFyYW0gewo+ ICAgIF9fdTY0IGF0dHI7Cj4gICAgX191NjQgYTsKPiAgICBfX3U2NCBiOwo+ICAgIF9fdTY0IGM7 Cj4gfTsKPiAKPiBHaXZlbiB0aGF0IHlvdSBuZWVkIGEgd3JhcHBlciB0byBzZXQgdGhlIHBvaW50 ZXIgaW4gbWVtcmVmIGFueXdheT8KPiAKPiBIYXZpbmcgYW4gaW9jdGwgd2l0aCBhIHZhcmlhYmxl IG51bWJlciBvZiB2YXJpYWJsZSB0eXBlIGFyZ3VtZW50cwo+IGlzIHJlYWxseSBhIHdlYWtuZXNz IG9mIHRoZSBBQkksIGJ1dCBJIGRvbid0IHNlZSBhIGdvb2Qgd2F5IGFyb3VuZAo+IGl0IGVpdGhl ciwgdGhlIGFib3ZlIHdvdWxkIGp1c3QgbWFrZSBpdCBzbGlnaHRseSBtb3JlIGRpcmVjdC4KCk9L LCBJJ2xsIGNoYW5nZS4KCj4gCj4gPiA+ID4gKy8qKgo+ID4gPiA+ICsgKiBzdHJ1Y3QgdGVlX2lv Y2xfc3VwcF9zZW5kX2FyZyAtIFNlbmQgYSByZXNwb25zZSB0byBhIHJlY2VpdmVkIHJlcXVlc3QK PiA+ID4gPiArICogQHJldDoJW291dF0gcmV0dXJuIHZhbHVlCj4gPiA+ID4gKyAqIEBudW1fcGFy YW1zCVtpbl0gbnVtYmVyIG9mIHBhcmFtZXRlcnMgZm9sbG93aW5nIHRoaXMgc3RydWN0Cj4gPiA+ ID4gKyAqLwo+ID4gPiA+ICtzdHJ1Y3QgdGVlX2lvY2xfc3VwcF9zZW5kX2FyZyB7Cj4gPiA+ID4g KwlfX3UzMiByZXQ7Cj4gPiA+ID4gKwlfX3UzMiBudW1fcGFyYW1zOwo+ID4gPiA+ICsJLyoKPiA+ ID4gPiArCSAqIHRoaXMgc3RydWN0IGlzIDggYnl0ZSBhbGlnbmVkIHNpbmNlIHRoZSAnc3RydWN0 IHRlZV9pb2N0bF9wYXJhbScKPiA+ID4gPiArCSAqIHdoaWNoIGZvbGxvd3MgcmVxdWlyZXMgOCBi eXRlIGFsaWdubWVudC4KPiA+ID4gPiArCSAqCj4gPiA+ID4gKwkgKiBDb21tZW50ZWQgb3V0IGVs ZW1lbnQgdXNlZCB0byB2aXN1YWxpemUgdGhlIGxheW91dCBkeW5hbWljIHBhcnQKPiA+ID4gPiAr CSAqIG9mIHRoZSBzdHJ1Y3QuIFRoaXMgZmllbGQgaXMgbm90IGF2YWlsYWJsZSBhdCBhbGwgaWYK PiA+ID4gPiArCSAqIG51bV9wYXJhbXMgPT0gMC4KPiA+ID4gPiArCSAqCj4gPiA+ID4gKwkgKiBz dHJ1Y3QgdGVlX2lvY3RsX3BhcmFtIHBhcmFtc1tudW1fcGFyYW1zXTsKPiA+ID4gPiArCSAqLwo+ ID4gPiA+ICt9IF9fYWxpZ25lZCg4KTsKPiA+ID4gCj4gPiA+IEknZCBtYWtlIHRoYXQgCj4gPiA+ IAo+ID4gPiAJc3RydWN0IHRlZV9pb2N0bF9wYXJhbSBwYXJhbXNbMF07Cj4gPiA+IAo+ID4gPiBh cyB3ZWwgaGVyZSwgYXMgSSBhbHNvIGNvbW1lbnRlZCBpbiBwYXRjaCAzIHRoYXQgaGFzIGEgc2lt aWxhciBzdHJ1Y3R1cmUuCj4gPiAKPiA+IEknbSBjb25jZXJuZWQgdGhhdCB0aGlzIG1heSBjYXVz ZSB3YXJuaW5ncyB3aGVuIGNvbXBpbGluZyBmb3IgdXNlciBzcGFjZQo+ID4gZGVwZW5kaW5nIG9u IGNvbXBpbGVyIGFuZCBvcHRpb25zLiBBbSBJIHRvbyBjYXV0aW91cyBoZXJlPwo+IAo+IFNlZSBo dHRwczovL2djYy5nbnUub3JnL29ubGluZWRvY3MvZ2NjL1plcm8tTGVuZ3RoLmh0bWwKPiAKPiBJ IGFjdHVhbGx5IG1pc3JlbWVtYmVyZWQgaXQgYW5kIHRoZSBzeW50YXggSSBsaXN0ZWQgaXMgR0ND IHNwZWNpZmljLAo+IGJ1dCBDOTkgYWxsb3dzICJmbGV4aWJsZSBhcnJheXMiLiBJIHRoaW5rIHRo ZXJlIGlzIG5vIHByb2JsZW0gcmVseWluZwo+IG9uIEM5OSBoZXJlLCB3ZSBhbHJlYWR5IHJlbHkg b24gQzk5IGZlYXR1cmVzIGVsc2V3aGVyZSBpbiBoZWFkZXJzLgoKT0ssIEknbGwgY2hhbmdlLgoK VGhhbmtzLApKZW5zCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3Rz LmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5m by9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750743AbdAWI2j (ORCPT ); Mon, 23 Jan 2017 03:28:39 -0500 Received: from mail-lf0-f53.google.com ([209.85.215.53]:36538 "EHLO mail-lf0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750703AbdAWI2i (ORCPT ); Mon, 23 Jan 2017 03:28:38 -0500 Date: Mon, 23 Jan 2017 09:20:02 +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: <20170123082001.GA1910@jax> References: <1479480700-554-1-git-send-email-jens.wiklander@linaro.org> <5825882.vZRDMrBMkW@wuerfel> <20170119164541.GA22094@jax> <8436100.O3a8W3D0ph@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8436100.O3a8W3D0ph@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 On Fri, Jan 20, 2017 at 05:47:40PM +0100, Arnd Bergmann wrote: > On Thursday, January 19, 2017 5:45:43 PM CET Jens Wiklander wrote: > > > > 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. > > Ok > > > > > > > > + 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 > > Ok, makes sense. > > > > > > > > +/** > > > > + * 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. > > Got it. I still think it's a bit awkward on the user ABI side. > I also see that (unlike the in-kernel interface) tee_ioctl_param_memref > and tee_ioctl_param_value are both defined in terms of three __u64 > members. Actually the last member of struct tee_ioctl_param_memref is an __s64, but that doesn't matter much one just have to be a little careful. > > How about simply using one format here and making this > > struct tee_ioctl_param { > __u64 attr; > __u64 a; > __u64 b; > __u64 c; > }; > > Given that you need a wrapper to set the pointer in memref anyway? > > Having an ioctl with a variable number of variable type arguments > is really a weakness of the ABI, but I don't see a good way around > it either, the above would just make it slightly more direct. OK, I'll change. > > > > > +/** > > > > + * 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? > > See https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html > > I actually misremembered it and the syntax I listed is GCC specific, > but C99 allows "flexible arrays". I think there is no problem relying > on C99 here, we already rely on C99 features elsewhere in headers. OK, I'll change. Thanks, Jens