Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page permissions

[Jarkko Sakkinen]

On Wed, Mar 02, 2022 at 02:57:45PM -0800, Reinette Chatre wrote:
> > What do you mean by "user space policy" anyway exactly? I'm sorry but I
> > just don't fully understand this.
> 
> My apologies - I just assumed that you would need no reminder about this contentious
> part of SGX history. Essentially it means that, yes, the kernel could theoretically
> permit any kind of access to any file/page, but some accesses are known to generally
> be a bad idea - like making memory executable as well as writable - and thus there
> are additional checks based on what user space permits before the kernel allows
> such accesses.

The device files are limited by a GID (in systemd upstream), which is a
"user policy".

What you want to add and why augmentation cannot be made complete before
the unknown factor is added to the access control?

> >>> I think the best way to move forward would be to do EAUG's explicitly with
> >>> an ioctl that could also include secinfo for permissions. Then you can
> >>> easily do the rest with EACCEPTCOPY inside the enclave.
> >>
> >> SGX_IOC_ENCLAVE_ADD_PAGES already exists and could possibly be used for
> >> this purpose. It already includes SECINFO which may also be useful if
> >> needing to later support EAUG of PT_SS* pages.
> > 
> > You could also simply add SGX_IOC_ENCLAVE_AUGMENT_PAGES and call it a day.
> 
> I could, yes.

And this enables EACCEPTCOPY pattern nicely.

E.g. you can implement mmap() with EAUG and then EACCEPTCOPY feeded with
permissions and a zero page:

1. enclave calls back to host to do mmap()
2. host does eaug on given range and enter back to enclave.
3. enclave does eacceptcopy with given permissions and a zero page.

> > I don't like this type of re-use of the existing API.
> 
> I could proceed with SGX_IOC_ENCLAVE_AUGMENT_PAGES if there is consensus after
> considering the user policy question (above) and performance trade-off (more below).

Ok.

If adding this would be a bottleneck it would be already persistent int
"add pages", so whatever limitation there might be, it already exist.

Thus, logically, that could be safely added without worrying about user
policies all that much...

> 
> > 
> >> The big question is whether communicating user policy after enclave initialization
> >> via the SECINFO within SGX_IOC_ENCLAVE_ADD_PAGES is acceptable to all? I would
> >> appreciate a confirmation on this direction considering the significant history
> >> behind this topic.
> > 
> > I have no idea because I don't know what is user space policy.
> 
> This discussion is about some enclave usages needing RWX permissions
> on dynamically added enclave pages. RWX permissions on dynamically added pages is

I'm not sure if that is actually necessary, if you use EAUG-EACCEPTCOPY
type of pattern. Please correct if I'm wrong.

> not something that should blindly be allowed for all SGX enclaves but instead the user
> needs to explicitly allow specific enclaves to have such ability. This is equivalent
> to (but not the same as) what exists in Linux today with LSM. As seen in
> mm/mprotect.c:do_mprotect_pkey()->security_file_mprotect() Linux is able to make
> files and memory be both writable and executable, but it would only do so for those
> files and memory that the LSM (which is how user policy is communicated, like SELinux)
> indicates it is allowed, not blindly do so for all files and all memory.

We could also potentially make LSM hooks to ioctls, if that is ever needed.

And as I said earlier, EAUG ioctl does not make things any worse they might
be.

> >>> Putting EAUG to the #PF handler and implicitly call it just too flakky and
> >>> hard to make deterministic for e.g. JIT compiler in our use case (not to
> >>> mention that JIT is not possible at all because inability to do RX pages).
> 
> I understand how SGX_IOC_ENCLAVE_AUGMENT_PAGES can be more deterministic but from
> what I understand it would have a performance impact since it would require all memory
> that may be needed by the enclave be pre-allocated from outside the enclave and not
> just dynamically allocated from within the enclave at the time it is needed.
> 
> Would such a performance impact be acceptable?

IMHO yes because bad behaving enclave can cause the same issue anyway,
and more indeterministic manner.

BR, Jarkko