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 ?

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

Why NOWAIT ?

What is the use of this member ?

Commit the style change of replacing space by tab now.

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.

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

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.

OK Thanks

I guess not needed. Thanks

Not needed, removed thanks.

Committed

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

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.

The asserts are easily triggered by user.

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.

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.

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

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

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

So why object->handle is an sgx_vm_handle ?

added a check against object ops.
thank you!

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

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 ?

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

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.

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

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

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

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

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

I want all this listed explicitly.

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

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

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

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

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)

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.

The comment is better placed before the function.

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

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