linux-kernel - Re: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel Software Guard Extensions

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]

Message-ID: <20171207160548.doovfdh2lqg5brm3@linux.intel.com>
Date:   Thu, 7 Dec 2017 18:05:48 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     "Christopherson, Sean J" <sean.j.christopherson@...el.com>
Cc:     "intel-sgx-kernel-dev@...ts.01.org" 
        <intel-sgx-kernel-dev@...ts.01.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Darren Hart <dvhart@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Shevchenko <andy@...radead.org>
Subject: Re: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for
 Intel Software Guard Extensions

On Thu, Dec 07, 2017 at 02:46:39PM +0000, Christopherson, Sean J wrote:
> > +	for (i = 0; i < 2; i++) {
> > +		va_page = list_first_entry(&encl->va_pages,
> > +					   struct sgx_va_page, list);
> > +		va_offset = sgx_alloc_va_slot(va_page);
> > +		if (va_offset < PAGE_SIZE)
> > +			break;
> > +
> > +		list_move_tail(&va_page->list, &encl->va_pages);
> > +	}
> 
> This is broken, there is no guarantee that the next VA page will have
> a free slot.  You have to walk over all VA pages to guarantee a slot
> is found, e.g. this caused EWB and ELDU errors.

I did run some extensive stress tests on this and did not experience any
issues. Full VA pages are always put to the end. Please point me to the
test where this breaks so that I can fix the issue if it persists.

> Querying list.next to determine if an encl_page is resident in the EPC
> is ugly and unintuitive, and depending on list's internal state seems
> dangerous.  Why not use a flag in the encl_page, e.g. as in the patch
> I submitted almost 8 months ago for combining epc_page and va_page into
> a union?  And, the encl's SGX_ENCL_SECS_EVICTED flag can be dropped if
> a flag is added to indicate whether or not any encl_page is resident in
> the EPC.
> 
> https://lists.01.org/pipermail/intel-sgx-kernel-dev/2017-April/000570.html

I think it is better to just zero list entry and do list_empty test. You
correct that checking that with poison is ugly.

Last flag bit wll be needed for the SGX_ENCL_PAGE_TRIM. It is useful to
have the flag in the enclave in order to be able to pack struct
sgx_encl_page.

/Jarkko