Hi Kai
On Thu, 16 Feb 2023 01:53:47 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
On Wed, 2023-02-15 at 09:42 -0600, Haitao Huang wrote:
On Wed, 15 Feb 2023 02:51:23 -0600, Huang, Kai <kai.huang@xxxxxxxxx>
wrote:
> > > > >
> > >
> > > Since sgx_mmap() can happen before enclave is created,
calculating the
> > > vm_pgoff
> > > from enclave_base is conceptually wrong. Even if you really want
to
> > do
> > > it, it
> > > should be:
> > >
> > > if (enclave_has_initialized())
> > > vma->vm_pgoff = ...;
> >
> > I got your point now. I can add a condition to test the
SGX_ENCL_CREATED
> > bit. However, we still have a hole if we must handle the sequence
> > mmap(..., enclave_fd) being called before ECREATE ioctl. We can't
leave
> > vm_pgoff not set for those cases.
> >
> > Since no one does that so far, can we explicitly return an error
from
> > sgx_mmap when that happens?
> > Other suggestions?
>
> As I replied to patch 4/4, I believe userspace should pass the correct
> pgoff in
> mmap(). It's wrong to always pass 0 or any random value.
>
> If userspace follow the mmap() rule, you won't need to manually set
> vm_pgoff
> here (which is hacky IMHO). Everything works fine.
>
SGX driver was following MAP_ANONYMOUS semantics. If we change that,
it'd
break current usage/ABI.
Is there any doc/reference mentioning this?
No, just from how we use it :-). Jarkko/Sean/Dave may have
history/insights on this?
But file offset in mmap spec implies there is a "real" file independent of
the memory it is mapped into, and you can mmap arbitrary offset of that
file to any memory address. That does not fit well with SGX enclave
memory: there is no separate physical file other than enclave residing in
memory and its base is fixed.
Again, my main concern is impact to existing usage.
I still think returning error for cases mmap(..., enclave_fd) if enclave
is not created would be less intrusive change.
But you need the first mmap() to work before IOCTL(ecreate) to get
enclave's
base address, correct?
No, mmap(..., MAP_ANONYMOUS, fd=-1) must be used to reserve address range
for enclave before calling IOCTL(ecreate). In fact, I realize mmap(...,
fd=enclave_fd) before ioctl(ecreate) should fail in sgx_encl_may_map
according to its definition but IIUC it would skip the loop xas_for_each
when no entries yet available in encl->page_array. This might be a hole to
enforcement of vma_prot_bits not exceeding the vm_max_prot_bits which is
recorded first during EADD.
So if my understanding is correct, we just need fix sgx_encl_may_map to
return -EACCES when mmap is called before ecreate is done.
Thanks
Haitao
