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

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Thu, 09 Mar 2023 05:31:29 -0600, Huang, Kai <kai.huang@intel.com> 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