Re: [RFC PATCH v4 2/4] x86/sgx: Implement support for MADV_WILLNEED

Huang, Kai
2023-03-09T11:31:58Z

On Wed, 2023-03-08 at 18:50 -0600, Haitao Huang wrote:
> Hi Kai
>
> Thanks for your detailed review.
>
> On Tue, 07 Mar 2023 17:32:15 -0600, Huang, Kai <kai.huang@intel.com> wrote:
>
> > On Tue, 2023-02-21 at 19:37 -0600, Haitao Huang wrote:
> > > On Tue, 21 Feb 2023 16:10:02 -0600, jarkko@kernel.org
> > > <jarkko@kernel.org>
> > > wrote:
> > >
> > > > 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@kernel.org
> > > > > <jarkko@kernel.org>
> > > > > 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@intel.com>
> > > > > > > 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.
> > > >
> > >
> > > Do you mean 'offset' passed in from mmap syscall and converted to pgoff
> > > in
> > > kernel? I can see it only passed to driver in get_unmapped_area. I can
> > > add
> > > this enforcement there so it is consistent to the MAP_ANONYMOUS spec if
> > > that's what you suggest.
> > >
> > > vma->vm_pgoff is the offset in pages of the vma->vm_start relative to
> > > 'file'.
> > > It can not be zero for all VMAs of the enclave.
> > >
> > > Kernel sets vma->vm_pgoff == vma->vm_start>>PAGE_SHIFT for private anon
> > > VMAs, and ignore it (zero) for shared anon mapping.
> > >
> > > For us, we propose to set it to PFN_DOWN(vma->vm_start - encl->base)
> > > upon
> > > mmap.
> >
> > Sorry for late reply. Basically I was sick and having limited working
> > time in
> > the passed two weeks.
> >
> > For what I understand now, SGX driver wants to use MAP_ANONYMOUS
> > semantic for
> > mmap()s against /dev/sgx_enclave (for whatever reason that I don't
> > understand).
> > And because of that, we even want to explicitly enforce userspace to
> > always pass
> > 0 as pgoff in mmap()s (hasn't been done yet).
> >
> > And then here, we want to rewrite vma->pgoff so that the VMA can act
> > _like_ a
> > normal file-based mmap() semantics (despite it is indeed a file-based
> > VMA),
> > because the VFS fadvice() implementation assumes file-based VMAs are
> > always
> > following file-based mmap() semantics.
> >
> > Hmm..
> >
> > Doesn't this sound weird?
> >
>
> Sometime weird means good or creative as expressed in a popular slogan in
> the city where I live. Just joking:-)
>
> > I must have been missing something, but why cannot SGX driver just
> > always follow
> > file-based mmap() semantics at the beginning?
> >
> > For instance, what's wrong with:
> >
> > 1) encl_fd = open("/dev/sgx_enclave")
> > 2) encl_base = mmap(..., encl_size, MAP_SHARED, encl_fd, 0 /* pgoff */)
> > 3) ioctl(ECREATE, encl_base, encl_size)
> >
> > (In ioctl(ECREATE), we might want to verify the VMA we found has the same
> > encl_base as starting address, and 0 pgoff).
> >
> > And in following mmap()s in which we want to map a small range of
> > enclave:
> >
> > encl_addr = mmap(encl_addr, MAP_SHARED|MAP_FIXED, encl_fd,
> > (encl_addr - encl_base) >> PAGE_SHIFT);
> >
> > ?
> >
> > Anything wrong above?
> >
>
> I believe this can be done except for that you are breaking existing code
> by requiring pg_off for all mmaps now.
>
> Doing mmap before ECREATE also effectively bypasses sgx_encl_may_map. And
> we have a very long thread of discussion about this[1] and that's why
> sgx_encl_may_map come into play to limit VMA permissions to those given in
> SecInfo by EADD ioctl. However, current code does not explicitly block it
> and I think it'd be better do it. More comment below on this point.

Thanks for the info!
>
> > In fact, if the first mmap() in step 2) before IOCTL(ecreate) used
> > MAP_ANONYMOUS, IIUC that VMA will stay as anonymous VMA but will never be
> > converted to SGX file-based one, because by looking at the code, I see
> > neither
> > ioctl(ECREATE) nor ioctl(EINIT) converts that VMA to file-based one.
> >
> > That being said, all page faults caused by userspace access to that VMA
> > will
> > still go to anonymous VMA's fault path, but not SGX driver's.
> >
>
> It is required to mmap anonymously with PROT_NONE initially and
> user space must invoke mmap with enclave fd after EADDing each
> segments/areas. PF handler will check VMA prot bits against
> vm_max_prot_bits in sgx_encl_load_page_in_vma, and report failure on any
> mismatch.
> If user space does not mmap(...,encl_fd) after EADD, pages will have
> PROT_NONE and fail on any access.
>
> > This is fine for SGX1 as all enclave pages are fully populated. For
> > SGX2,
> > _unless_ you explicitly mmap("/dev/sgx_enclave") those dynamical ranges,
> > the
> > fault will never be handled by SGX driver.
> >
>
> Indeed, it is required user space invoke mmap to config the regions for
> EAUG.
>
> > Architecturally, for SGX2, if you mmap() the entire enclave range (as in
> > step
> > 2), you don't need explicitly mmap() all dynamic ranges. The userspace
> > should
> > just be able to access those dynamic ranges (i.e. EACCEPT) and the
> > kernel driver
> > should gracefully handle the fault.
> >
> > Shouldn't this be a problem?
> >
> > (Btw, there might be other corner cases that could cause VMA
> > splitting/merging,
> > etc, but I haven't thought about those)
> >
> > > The concern Kai raised about encl->base not available in the window
> > > between mmap and ECREATE can be addressed by disallowing mmap before
> > > ECREATE is done. It does not make much sense anyway to mmap(enclave_fd)
> > > without a valid enclave range associated with enclave_fd.
> > >
> >
> > We can, but I don't understand why the first mmap() before ECREATE
> > must/should
> > be done via MMAP_ANONYMOUS. In fact, I think it might be wrong for SGX2.
> >
>
> There is a long history behind it. The use of anonymous mapping can be
> traced back to v29.
> See this thread[2].
>
> Before v29, user space can do mmap(PROT_NONE, ..., enclave_fd) before
> ECREATE to reserve a range for the enclave. But it must be PROT_NONE for
> the reservation, otherwise sgx_encl_may_map will block it by the "!page"
> check below [3]:
>
> int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> unsigned long end, unsigned long vm_prot_bits)
> {
> unsigned long idx, idx_start, idx_end;
> struct sgx_encl_page *page;
>
> /* PROT_NONE always succeeds. */
> if (!vm_prot_bits)
> return 0;
>
> idx_start = PFN_DOWN(start);
> idx_end = PFN_DOWN(end - 1);
> for (idx = idx_start; idx <= idx_end; ++idx) {
> mutex_lock(&encl->lock);
> page = radix_tree_lookup(&encl->page_tree, idx);
> mutex_unlock(&encl->lock);
>
> if (!page || (~page->vm_prot_bits & vm_prot_bits))
> return -EACCES;
> ...
>
> Then in v29, PROT_NONE mapping was disallowed for encl_fd before pages
> EADDed so user space has to mmap anonymously to reserve the range.The
> intent was still not to allow mmap before pages EADDed (the !page check
> was still there up to v38)

Do you know the reason of disallowing PROT_NONE mapping against encl_fd?

I dig v28 roughly but didn't find any clue.

Basically no comments related to this were made to:

[PATCH v28 11/22] x86/sgx: Linux Enclave Driver

>
> Later version (around v39?) we switched to enforce the permissions in PF
> handler and mmap with any permissions before EADD is allowed, but may
> cause failure later on PF. I'm not sure it was intentional but pretty sure
> no meaningful usage for doing mmap before ECREATE due to PF handler
> enforcement.

IIUC this change around v39 re-allows mmap() against encl_fd before ECREATE?

IIUC the only enforcement is VMA's permission must be more restricted than
enclave page's permission.

So, theoretically, we can mmap() encl_fd with PROT_READ|PROT_WRITE|PROT_EXEC,
and then mprotect() the address range based on the actual enclave pages (in
EADD) before actually doing EADD those pages?

>
> I think that's the history behind it. Others more knowledgeable can
> correct me as needed.

Thanks for the information.

>
> Again, even if we redesign the whole thing to be less "weird", then
> requiring pgoff for each mmap would break existing user space code. And I
> think explicitly block mmap before ECREATE make the API more consistent
> with the PF handler enforcement and original intent of sgx_encl_may_map.

I think permission check in sgx_encl_may_map() isn't restricted related to the
vma->pgoff issue here. They are basically two different topics, IIUC.

So I am still a little bit confused about where does "SGX driver uses
MAP_ANONYMOUS semantics for fd-based mmap()" come from.

Anyway, we certainly don't want to break userspace. However, IIUC, even from
now on we change the driver to depend on userspace to pass the correct pgoff in
mmap(), this won't break userspace, because old userspace which doesn't use
fadvice() and pgoff actually doesn't matter. For new userspace which uses
fadvice(), it needs to pass the correct pgoff.

I am not saying we should do this, but it doesn't seem we can break userspace?

>
> Thanks again for detailed review and those thoughtful questions.
>
> Haitao
>
> [1]https://lore.kernel.org/linux-sgx/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com/
> [2]https://lore.kernel.org/linux-sgx/CAOASepPFe_ucuwe7JW_-+VBQ4=+sHqyGXOdA9kUbcYA_9=v0sA@mail.gmail.com/
> [3]https://lore.kernel.org/linux-sgx/20190713170804.2340-17-jarkko.sakkinen@linux.intel.com/

Thanks again for digging out those links. I haven't read the entire discussion,
though, as they are pretty long.