On Thu, Jun 29, 2023, Kai Huang wrote:
> On Wed, 2023-06-28 at 07:57 -0700, Sean Christopherson wrote:
> > On Wed, Jun 28, 2023, Kai Huang wrote:
> > > (but requires MAP_FIXED).
> >
> > No, SGX doesn't require MAP_FIXED. The requirements of ELRANGE make it extremely
> > unlikely that mmap() will succeed, but it's not a strict requirement.
>
> Looks w/o MAP_FIXED, the driver just uses the generic mm->get_unmapped_area() to
> return the address, which doesn't guarantee the right address will be returned
> at all. Especially when ELRANGE is reserved via mmap(NULL), the
> mmap(/dev/sgx_enclave) will not return the correct address no matter what pgoff
> is used IIUC.
>
> static unsigned long sgx_get_unmapped_area(struct file *file,
> unsigned long addr,
> unsigned long len,
> unsigned long pgoff,
> unsigned long flags)
> {
> if ((flags & MAP_TYPE) == MAP_PRIVATE)
> return -EINVAL;
>
> if (flags & MAP_FIXED)
> return addr;
>
> return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> }
>
> So to me userspace has to use MAP_FIXED to get the correct address.
No. As called out below, @addr is still used as a fairly strong hint:
unsigned long
arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
const unsigned long len, const unsigned long pgoff,
const unsigned long flags)
{
struct vm_area_struct *vma;
struct mm_struct *mm = current->mm;
unsigned long addr = addr0;
struct vm_unmapped_area_info info;
/* requested length too big for entire address space */
if (len > TASK_SIZE)
return -ENOMEM;
/* No address checking. See comment at mmap_address_hint_valid() */
if (flags & MAP_FIXED)
return addr;
/* for MAP_32BIT mappings we force the legacy mmap base */
if (!in_32bit_syscall() && (flags & MAP_32BIT))
goto bottomup;
/* requesting a specific address */ <==================================
if (addr) {
addr &= PAGE_MASK;
if (!mmap_address_hint_valid(addr, len))
goto get_unmapped_area;
vma = find_vma(mm, addr);
if (!vma || addr + len <= vm_start_gap(vma))
return addr;
}
...
}
In practice, I expect most/all userspace implementations do use MAP_FIXED, but
it's not strictly required.
> > > 2) Therefore,�"passing the correct pgoff" in new userspace app doesn't break the
> > > current ABI. If the new app chooses to pass the correct pgoff, it will work.
> >
> > Yep.
> >
> > > 3) With additional support of sgx_fadvice(WILLNEED) within the driver, the new
> > > app can use madvice(WILLNEED) if it passes the correct pgoff when mmap(). If
> > > wrong pgoff is passed, then madvice(WILLNEED) will work unexpectedly and could
> > > result in enclave being killed. It's userspace app's responsibility to make
> > > sure the correctness, not the driver's.
> >
> > Hmm, yeah, it's probably ok to lean on documentation if userspace screws up. I
> > generally prefer explicit errors over "undefined behavior", but I don't have a
> > super strong opinion, especially since this isn't my area these days :-)
>
> My argument is for normal file operations, if you use madvice(WILLNEED) but
> didn't mmap() with the correct pgoff, you will end up with kernel acting on the
> wrong portion of the file (which may not result in fatal error, but certainly
> not the correct thing from userspace's perspective).
>
> So kernel doesn't guarantee anything here, but depends on userspace to do things
> correctly.
Fair enough.
> > > 4) Old SGX apps which don't use file-based ABIs and still pass 0 pgoff should
> > > continue to work. No break of old apps either.
> >
> > Yep.
> >
> > > 5) We encourage new apps to always pass the correct pgoff instead of passing 0.
> > >
> > > 6) If needed, we can modify sgx_mmap() to relax the needing to use MAP_FIXED,
> > > but return the enclave's address "based on pgoff from userspace".
> >
> > As above, this isn't a relaxation of anything since MAP_FIXED isn't strictly
> > required, it's simply additional functionality.
> >
> > > This effectively provides additional mmap() ABI for userspace while not
> > > breaking the existing MAP_FIXED ABI.
> >
> > I don't think you want to do this. The MAP_FIXED case won't be affected, but the
> > address provided to mmap() is also used as a hint in the !MAP_FIXED case,, i.e.
> > MAP_FIXED just means use *exactly* this address and don't try anything else. It's
> > unlikely, but not _that_ unlikely, that there are userspace applications that
> > specify the exact address without MAP_FIXED and without the correct pgoff.
