Intel SGX driver

[br]

Summary

Kernel support for Intel SGX.

Project wiki:
https://wiki.freebsd.org/Intel_SGX

Test Plan

I tested this in QEMU-SGX under linux with kvm-sgx and SMP support enabled.
QEMU-SGX provides 32 MB of EPC memory.

Also I tested this on Lenovo ThinkPad X1 Carbon 2017 (5th gen) with FreeBSD natively with SMP enabled.

Test involves creating/removing 10-50 enclaves in parallel using sample application called LocalAttestation.

[br]

prevent repeated enclave creation for the same object: check if page was not inserted to object. Same for adding page ioctl

[kib]

I did mostly a cursory look over the new code. First, I admit that now it is much more understandable. But please see the question I asked inline in sgx_ioctl_create() which blocks my further understanding of the code.

sys/amd64/sgx/sgx.c

552

This is racy, the object might be terminated before you get to this _reference() call.

I still do not understand how enclaves are created. My current theory is that the structure of the driver suggests that userspace needs to do mmap("/dev/sgx") and then issue SGX_IOC_ENCLAVE_CREATE on that mapping. Am I close to the right ?

If some bits from above are true, what prevents user from calling the ioctl on a mapping which was not created my mmaping /dev/sgx ? Also, as I remember from the SGX docs, the alignment of the userspace mapping is somewhat strict, does driver need to enforce this ?

895

This needs to be done under the object wlock, and I recommend to use vm_object_set_flag().

934

Why NOWAIT ?

sys/amd64/sgx/sgxvar.h

75 ↗

(On Diff #30872)

What is the use of this member ?

sys/vm/vm_object.h

177 ↗

(On Diff #30872)

Commit the style change of replacing space by tab now.

178 ↗

(On Diff #30872)

This should be extracted into separate review with me, markj and alc set as reviewers.

You may consider covering the TAILQ_FOREACH_SAFE() loop right before the check for resident_page_count in vm_object_terminate(), as well.

sys/vm/vm_page.c

1213 ↗

(On Diff #30872)

This will break 32bit arches. Use %ju/cast to uintmax_t.

[br]

• cleanup the object correctly with vm_page_remove()
• remove unused struct member 'dev'
• use WAITOK in sgx_get_epc_area()
• vm_object patch gone to separate review

[br]

• reference the object before vm_map_unlock_read()
• use vm_object_set_flag()

[br]

• check if secs->base is naturally aligned on an secs->size boundary
• check if secs->size is at least 2 pages

[br]

• fix struct secinfo as it requires 64 bytes alignment, not 128 (based on EADD operation page of manual)

[br]

sys/amd64/sgx/sgx.c

552

I moved object_reference() call before vm_map_unlock_read(). Should be OK now ?

Yes, user calls mmap("/dev/sgx") and then issue SGX_IOC_ENCLAVE_CREATE on that mapping to create SECS page.
Nothing prevents user from doing that, but we proceed SECS struct validation (SECS struct provided by user with ENCLAVE_CREATE ioctl), including size and alignments checks, etc. User must provide SECS->base address he get from mmap /dev/sgx

We can also set some flag to vmh in sgx_mmap_single(() to ensure later that mapping we found based on address specified was created by mmap /dev/sgx ?

We enforce alignment since malloc returns PAGE_SIZE aligned pointers so pginfo.srcpge is 4K aligned for both CREATE and ADD_PAGE ioctl calls, sigstruct and einittoken are both properly aligned. Also struct page_info is aligned(32), secinfo is aligned(64), etc.

We may also check that initp->addr and addp->addr are both aligned, but if not, then we just find no enclave and return error to user on ADD_PAGE and INIT calls.

895

OK Thanks

934

I guess not needed. Thanks

sys/amd64/sgx/sgxvar.h

75 ↗

(On Diff #30872)

Not needed, removed thanks.

sys/vm/vm_object.h

177 ↗

(On Diff #30872)

Committed

178 ↗

(On Diff #30872)

OK I added review
https://reviews.freebsd.org/D11688

[kib]

sys/amd64/include/cpufunc.h

833

I thought some about the SGX instructions invocations, which was related to the note about missed check for enclave base address alignment. I think that the latest patch does not check all conditions that are listed in current version of SDM as invalid, e.g. there is no check that user passed the canonical address as the base. I believe there is more.

Which makes me suggest that instead of trying to find all such conditions and cover them, system should be prepared for #gp from sgx instructions. Look e.g. at the rdmsr_sage/wrmsr_safe functions from amd64/support.S which were written for cpuctl(4) to provide safe access to MSR, which also #gp-s on invalid MSR number or content.

sys/amd64/sgx/sgx.c

226

The asserts are easily triggered by user.

573

Note that userspace might access the mmaped region before or during the enclave create ioctl is run. How this situation is handled ?

Also malicious or buggy userspace can unman the mapped region.

[br]

• be ready for #gp faults caused by SGX instructions. Fault checks added for ecreate, eadd, einit and eextend

[br]

• fix vm page removal code: use TAILQ_FOREACH_SAFE(p, &object->memq,...)

[kib]

Can you provide a comment at the start of the sgx.c giving a short overview of the design and specifically listing ofsets for all control structure pages inside the enclave vm object ? Also please explain locking, i.e. the name mtx is not very descriptive for the lock purpose.

sys/amd64/sgx/sgx.c

226

This is still true. And I think that I already asked this, much earlier. Why do you trust user that init() is only called on mapping of /dev/sgx ? The object you get there may be completely unrelated, i.e. the mem->handle is not necessary an enclave.

930

The object variable is rather pointless. Also you still did not added wlock around vm_object_set_flag().

1024

make_dev(9) in fact does not fail. It panics on failure.

sys/amd64/sgx/sgx_support.S

50 ↗

(On Diff #31311)

Do you need the PCB_ONFAULT set to sgx_onfault during registers moves ? Put it right before the encls instruction.

[br]

rework sgx_mem_find()

[br]

move sgx_onfault just before encls

[br]

• add lock around vm_object_set_flag in sgx_mmap_single()
• remove unneeded object variable in sgx_mmap_single()
• make_dev cannot fail

[kib]

sys/amd64/sgx/sgx.c

233

So why object->handle is an sgx_vm_handle ?

[br]

check if object ops == &sgx_pg_ops in sgx_mem_find()

[br]

sys/amd64/sgx/sgx.c

233

added a check against object ops.
thank you!

[kib]

sys/amd64/sgx/sgx.c

206

Too many (). I think the check is fine now.

[br]

• remove excessive braces in sgx_get_epc_area()
• remove excessive parentheses in sgx_mem_find()

[br]

• Add driver design overview
• Remove enclave->mtx (unused)
• Rename sc->mtx to sc->mtx_encls (use around ENCLS instructions only)
• Add sc->mtx for sc->enclaves and sc->state access

[kib]

sys/amd64/sgx/sgx.c

80

So does it mean that user can access the SECS page ?

I expected that control pages not needed to be mapped to user would be indexed outside the [0, size) range. I was not able to easily reconcile the driver code and this suggestion, so I asked to explicitly list indexes.

There are other auxillary pages as well, are they inserted into the object queue ? If yes, at which indexes ?

109

I believe a newer version of this text is included into SDM.

[kib]

sys/amd64/sgx/sgx.c

595

I do not think that this printf is useful other than for debugging.
Also, a comment explaining that the trivial handler purpose is to handle the race and description of the race, would be useful.

[br]

• Use special index SGX_SECS_VM_OBJECT_INDEX (-1) in VM object queue for SECS page
• Change link to SDM (instead of old SGX manual dated 2014)
• printf is for debugging only in sgx_pg_fault

[br]

sys/amd64/sgx/sgx.c

80

I think you right: user can't access SECS page. SECS page can be accessed by hardware only.
So I now use special index SGX_SECS_VM_OBJECT_INDEX (-1) for SECS page.
We are also using special indexes for VA (version array) pages.
We don't use special index for TCS, because some fields (flags) of this page can be accesses by software.
No other auxillary pages inserted to VM object queue.

thanks

109

Right, thanks. I never seen that. But it look new (2017). So I changed the link pointing to SDM

595

I changed this to dprintf.
Not sure yet what the race you talk about ?
thanks!

[br]

sys/amd64/sgx/sgx.c

595

Do you mean the race when user can access VM object pages before IOCTL CREATE ? or during IOCTL_CREATE

[br]

• Change p2 in the enclave lifecycle overview: indicate SECS pages stored at special index SGX_SECS_VM_OBJECT_INDEX (-1) in enclave's VM object queue.

[kib]

sys/amd64/sgx/sgx.c

80

So what are the indexes for other pages ? What is the index for TCS ?

I want all this listed explicitly.

595

Yes, that race. I do not know any other reason to have this handler. Are they ?

[br]

sys/amd64/sgx/sgx.c

80

The offset to TCS page specified by user (addp->addr). Enclave can have more than one TCS page (one TCS per each thread).

[br]

sys/amd64/sgx/sgx.c

80

So we have 5 pages types:

1. PT_SECS index is SGX_SECS_VM_OBJECT_INDEX (-1)
2. PT_TCS index specified by user
3. PT_REG index specified by user
4. PT_VA index is special, created per each PT_REG, PT_TCS and PT_SECS page and determined by formula:

va_page_idx = - SGX_VA_PAGES_OFFS - (page_idx / SGX_VA_PAGE_SLOTS);

5. PT_TRIM is unused

[br]

• Sanity check the page we are adding is TCS or REG. We can't add other types of pages in IOCTL SGX_ADD_PAGE

[br]

• Add simple comment in sgx_pg_fault

[br]

• Style: remove trailing space

[br]

sys/amd64/sgx/sgx.c

80

I added a check that only TCS and REG pages can be added by user in ADD_PAGE ioctl

595

The only reason to have sgx_pg_fault is to return VM_PAGER_FAIL before pages added (i.e. before IOCTL create/add_page is run)

[kib]

sys/amd64/sgx/sgx.c

80

Still, I do want to see the list of all special pages and their offsets in the vm object, regardless of who adds them. If some kind of pages are added at user specified offset, say so and indicate where the offset if specified.

611

The comment is better placed before the function.

[br]

• Add a description of EPC page indexes in VM object queue

[alc]

sys/amd64/sgx/sgx.c

190–191

I would strongly suggest using a vmem arena to implement EPC allocation and deallocation.

[br]

Allocate EPC memory using vmem(9).

[br]

• Destroy vmem on driver unload

[br]

sys/amd64/sgx/sgx.c

190–191

Thanks I changed a way of EPC memory allocation in favor of using vmem(9).

[br]

• Fix printf on vmem arena creation failure
• Rename epc0 to epc in sgx_get_epc_page()