Re: [PATCH v20 00/28] Intel SGX1 support

[Andy Lutomirski]

> On Apr 19, 2019, at 2:56 PM, Jethro Beekman <jethro@xxxxxxxxxxxx> wrote:
>
>> On 2019-04-19 14:34, Thomas Gleixner wrote:
>> And how so? You create writeable AND executable memory. That's a nono and
>> you can argue in circles, that's not going to change with any of your
>> proposed changes.
>
>> On 2019-04-19 14:38, Thomas Gleixner wrote:
>> You are working around LSM nothing else and that's just not going to fly.
>
> Based on your comments, I'm still unsure if we're on the same page with
> regards to what I'm proposing.
>
> Here's a regular non-SGX flow that LSM would likely prevent:
>
> mmap(PROT_READ|PROT_WRITE)
> memcpy()
> mmap(PROT_READ|PROT_EXEC) <-- denied by LSM
>
> Or just something based on regular PT permissions:
>
> mmap(PROT_READ|PROT_EXEC)
> memcpy() <-- SIGSEGV
>
> Now, the equivalent for SGX:
>
> mmap(PROT_READ|PROT_WRITE)
> ioctl(EADD)
> mmap(PROT_READ|PROT_EXEC) <-- denied by LSM
>
> This works fine with v20 as-is. However, consider the equivalent of the
> PT-based flow:
>
> mmap(PROT_READ|PROT_EXEC)
> ioctl(EADD) <-- no error!

Indeed!

>
> It's not me that's working around the LSM, it's the SGX driver! It's
> writing to memory that's not marked writable! The fundamental issue here
> is that the SGX instruction set has several instructions that bypass the
> page table permission bits, and this is (naturally) confusing to any
> kind of reference monitor like the LSM framework. You can come up with
> similar scenarios that involve PROT_READ|PROT_WRITE|PROT_EXEC or
> ptrace(PTRACE_POKETEXT). So, clearly, the proper way to fix this failure
> of complete mediation is by enforcing appropriate page-table permissions
> even on the SGX instructions that don't do it themselves. Just make any
> implicit memory access look like a regular memory access and now
> everyone is on the same page (pun intended).
>

I agree that we should do this.  But then what?  Once this gets fixed,
the ioctl(EADD) fails and the driver becomes rather less useful, and
we feel rather silly if weâve merged it in this state.

So I think we need a better EADD ioctl that explicitly does work on
PROT_READ|PROT_EXEC enclave memory but makes up for by validating the
*source* of the data.  The effect will be similar to mapping a
labeled, appraised, etc file as PROT_EXEC.  Maybe, in extreme
pseudocode:

fd = open(â/dev/sgx/enclaveâ);
ioctl(fd, SGX_CREATE_FROM_FILE, file_fd);
// fd now inherits the LSM label from the file, or is otherwise marked.
mmap(fd, PROT_READ|PROT_EXEC, ...);

I suppose that an alternative would be to delegate all the EADD calls
to a privileged daemon, but thatâs nasty.