On Wed, 2023-06-28 at 07:57 -0700, Sean Christopherson wrote:
> On Wed, Jun 28, 2023, Kai Huang wrote:
> > On Tue, 2023-06-27 at 07:50 -0700, Sean Christopherson wrote:
> > > The question that you really want to ask is "could change XYZ break
> > > userspace?"
> >
> > Agreed.
> >
> > But since "encl->has_mismatched_offsets" is sort of new ABI, I think we need to
> > be careful otherwise in the future we may hit this kinda nasty issue again.
> >
> > Here's my thoughts:
> >
> > (Again, let's forget about mmap(/dev/sgx_enclave) to reserve ELRANGE for now.)
> >
> > 1) Current ABI is SGX driver _ignores_ pgoff for mmap(/dev/sgx_enclave)s
>
> Yes.
>
> > (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.
>
> > 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.
>
> > 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.
>
> If there were more to gain by steering mmap() based solely on pgoff, then it might
> be worth trying, but this doesn't seem to provide much value to userspace since
> the virtual address of any given enclave mapping is mission critical, e.g. it seems
> unlikely that userspace wouldn't already know the virtual address it needs.
I think we can forget about this discussion right now, because it isn't strictly
related to the problem that we want to clarify at this moment. My bad to bring
it up :)
