On Thu, 09 Mar 2023 05:31:29 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
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 think it was to allow user space to do anonymous mapping to reserve
address space for enclave.
Before this point, you have to use PROT_NONE to reserve with encl_fd.
There might be an issue with how #PF and EPC swapping was handled or the
elegance of those flows that motivated the move but I can't remember. ABI
was not fixed at that time so it was OK to change.
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?
I don't think so. There is no meaningful usage for that as I said.
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.
They are related because the intention was not allowing user space to do
arbitrary mmaping before EADD.
So in the context of the gap you identified for this patch series, i.e,
mmap before ECREATE, it is relevant. My view is that we never intended and
there is no use case for allowing that to happen. So it naturally follows
that we fix that gap by not allowing those scenarios explicitly.
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?
I'll leave it for others to chime in on this.
With my user space developer hat on, I would not like this because in one
place (mmap before madvise call) I have to pass pgoff and not in another
place (mmap after EADD). If you see really big issues on the current
MAP_ANONYMOUS + enclave_fd semantics, then we should fix the design. But
it should be in separate patches.
Thanks
Haitao
