linux-kernel - Re: [PATCH v39 11/24] x86/sgx: Add SGX enclave driver

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Oct 2020 06:06:19 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     x86@...nel.org, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jethro Beekman <jethro@...tanix.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>,
        Chunyang Hui <sanqian.hcy@...fin.com>,
        Jordan Hand <jorhand@...ux.microsoft.com>,
        Nathaniel McCallum <npmccallum@...hat.com>,
        Seth Moore <sethmo@...gle.com>,
        Darren Kenny <darren.kenny@...cle.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Suresh Siddha <suresh.b.siddha@...el.com>,
        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, puiterwijk@...hat.com,
        rientjes@...gle.com, tglx@...utronix.de, yaozhangx@...gle.com,
        mikko.ylinen@...el.com
Subject: Re: [PATCH v39 11/24] x86/sgx: Add SGX enclave driver

On Mon, Oct 05, 2020 at 02:30:53AM +0100, Matthew Wilcox wrote:
> > In my Geminilake NUC the maximum size of the address space is 64GB for
> > an enclave, and it is not fixed but can grow in microarchitectures
> > beyond that.
> > 
> > That means that in (*artificial*) worst case the locks would be kept for
> > 64*1024*1024*1024/4096 = 16777216 iterations.
> 
> Oh, there's support for that on the XArray API too.
> 
>         xas_lock_irq(&xas);
>         xas_for_each_marked(&xas, page, end, PAGECACHE_TAG_DIRTY) {
>                 xas_set_mark(&xas, PAGECACHE_TAG_TOWRITE);
>                 if (++tagged % XA_CHECK_SCHED)
>                         continue;
> 
>                 xas_pause(&xas);
>                 xas_unlock_irq(&xas);
>                 cond_resched();
>                 xas_lock_irq(&xas);
>         }
>         xas_unlock_irq(&xas);

Assuming we can iterate the array without encl->lock, I think this
would translate to:

/*
 * Not taking encl->lock because:
 * 1. page attributes are not written.
 * 2. the only page attribute read is set before it is put to the array
 *    and stays constant throughout the enclave life-cycle.
 */
xas_lock(&xas);
xas_for_each_marked(&xas, page, idx_end) {
	if (++tagged % XA_CHECK_SCHED)
		continue;

	xas_pause(&xas);
	xas_unlock(&xas);

	/*
	 * Attributes are not protected by the xa_lock, so I'm assuming
	 * that this is the legit place for the check.
	 */
	if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
		return -EACCES;

	cond_resched();
 	xas_lock(&xas);
}
xas_unlock(&xas);

Obviously, we cannot use this pattern by taking the encl->lock inside
the loop (ABBA and encl->lock is a mutex).

Let's enumerate:

A. sgx_encl_add_page(): uses xa_insert() and xa_erase().
B. sgx_encl_load_page(): uses xa_load().
C. sgx_encl_may_map(): is broken (for the moment).

A and B implicitly the lock and if a page exist at all we only access
a pure constant.

Also, since the open file keeps the instance alive, nobody is going
to pull carpet under our feet.

OK, I've just concluded tha we don't need to take encl->lock in this
case. Great.

/Jarkko