linux-kernel - Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Nov 2020 09:54:47 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Hillf Danton <hdanton@...a.com>,
        Jarkko Sakkinen <jarkko@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Jarkko Sakkinen <jarkko@...nel.org>,
        Jethro Beekman <jethro@...tanix.com>,
        akpm@...ux-foundation.org, andriy.shevchenko@...ux.intel.com,
        asapek@...gle.com, bp@...en8.de, cedric.xing@...el.com,
        chenalexchen@...gle.com, conradparker@...gle.com,
        cyhanish@...gle.com, dave.hansen@...el.com, haitao.huang@...el.com,
        kai.huang@...el.com, kai.svahn@...el.com, kmoy@...gle.com,
        ludloff@...gle.com, luto@...nel.org, nhorman@...hat.com,
        npmccallum@...hat.com, puiterwijk@...hat.com, rientjes@...gle.com,
        tglx@...utronix.de, yaozhangx@...gle.com, mikko.ylinen@...el.com
Subject: Re: [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

Hillf, I noticed that you removed a bunch of folks from cc, including
me.  Was there a reason for that?  I haven't been seeing your feedback
on these patches at all.

On 11/14/20 8:40 PM, Hillf Danton wrote:
> On Fri, 13 Nov 2020 00:01:23 +0200 Jarkko Sakkinen wrote:
>> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct sgx_encl *encl = filep->private_data;
>> +	int ret;
>> +
>> +	if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags))
>> +		return -EBUSY;
> Looks like encl->ioctl_mutex is needed to exlusively serialize
> concurrent ioctl threads and make encl->flags free of the duty.
> Plus it can cut the confusing EBUSY in userspace as it is not
> a critical path in any form.

I actually think the -EBUSY might be a bit too nice.  This is a *bad*
condition that userspace shouldn't be hitting.

Sean had a great explanation of this in a private mail:

> Reclaiming (i.e. swapping) pages out of an enclave can be done while an enclave
> is being built.  Reclaiming involves ENCLS, which needs to be serialized for a
> given enclave, i.e. reclaiming pages needs to take encl->lock.  To help adjust
> to EPC pressure, reclaim is automatically performed when allocating an EPC page,
> i.e. is triggered by ioctls.  Holding encl->lock for the entire ioctl() will
> thus deadlock if an enclave reclaims from itself.
> 
> There are other ways to solve the deadlock problem, e.g. only reclaim from
> workers and never from process context, but there are other motivations for
> the atomic shenanigans (see below).

> ENCLS instructions must be serialized for a given enclave, but holding
> encl->lock for an entire ioctl() will result in deadlock due to an enclave
> triggering reclaim on itself.
> 
> Building an enclave must also be serialized, i.e. userspace can't queue up
> EADD on multiple threads, because the order in which pages are added to an
> enclave affects the measurement.  In other words, rejecting the ioctl() as
> opposed to waiting on a lock is also desirable.

Sounds like we need should follow up with an add-on patch to get some of
that into a comment.