On Fri, Feb 17, 2023 at 05:03:05PM -0600, Haitao Huang wrote: > Hi Jarkko > > On Fri, 17 Feb 2023 16:32:12 -0600, jarkko@xxxxxxxxxx <jarkko@xxxxxxxxxx> > wrote: > > > On Wed, Feb 15, 2023 at 09:42:30AM -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. > > > > > > I still think returning error for cases mmap(..., enclave_fd) if > > > enclave is > > > not created would be less intrusive change. > > > > Is this something you care in SGX SDK? > > > > It is not categorically forbidden so that's why I'm asking. > > SDK does not care as we would never do this and don't think anyone is doing > that either. Suggesting returning error to cover all cases so user space > would not accidentally cause incorrect vm_pgoff set. vm_pgoff != 0 should result -EINVAL. BR, Jarkko
