Hi all,
On Wed, 02 Mar 2022 16:57:45 -0600, Reinette Chatre
<reinette.chatre@xxxxxxxxx> wrote:
Hi Jarkko,
On 3/1/2022 6:05 PM, Jarkko Sakkinen wrote:
On Tue, Mar 01, 2022 at 09:48:48AM -0800, Reinette Chatre wrote:
Hi Jarkko,
On 3/1/2022 5:42 AM, Jarkko Sakkinen wrote:
With EACCEPTCOPY (kudos to Mark S. for reminding me of this version
of
EACCEPT @ chat.enarx.dev) it is possible to make R and RX pages but
obviously new RX pages are now out of the picture:
/*
* Adding a regular page that is architecturally allowed to only
* be created with RW permissions.
* TBD: Interface with user space policy to support max permissions
* of RWX.
*/
prot = PROT_READ | PROT_WRITE;
encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
encl_page->vm_max_prot_bits = encl_page->vm_run_prot_bits;
If that TBD is left out to the final version the page augmentation
has a
risk of a API bottleneck, and that risk can realize then also in the
page
permission ioctls.
I.e. now any review comment is based on not fully known territory,
we have
one known unknown, and some unknown unknowns from unpredictable
effect to
future API changes.
The plan to complete the "TBD" in the above snippet was to follow this
work
with user policy integration at this location. On a high level the
plan was
for this to look something like:
/*
* Adding a regular page that is architecturally allowed to only
* be created with RW permissions.
* Interface with user space policy to support max permissions
* of RWX.
*/
prot = PROT_READ | PROT_WRITE;
encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
if (user space policy allows RWX on dynamically added pages)
encl_page->vm_max_prot_bits = calc_vm_prot_bits(PROT_READ |
PROT_WRITE | PROT_EXEC, 0);
else
encl_page->vm_max_prot_bits = calc_vm_prot_bits(PROT_READ |
PROT_WRITE, 0);
The work that follows this series aimed to do the integration with user
space policy.
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.
For example,
mm/mprotect.c:do_mprotect_pkey()->security_file_mprotect()
User policy and SGX has seen significant discussion. Some notable
threads:
https://lore.kernel.org/linux-security-module/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@xxxxxxxxxxxxxx/
https://lore.kernel.org/linux-security-module/20190619222401.14942-1-sean.j.christopherson@xxxxxxxxx/
It's too big of a risk to accept this series without X taken care of.
Patch
series should neither have TODO nor TBD comments IMHO. I don't want to
ack
a series based on speculation what might happen in the future.
ok
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 if there is plan to extend SGX_IOC_ENCLAVE_ADD_PAGES what is this
weird
thing added to the #PF handler? Why is it added at all then?
I was just speculating in my response, there is no plan to extend
SGX_IOC_ENCLAVE_ADD_PAGES (that I am aware of).
How this could work is user space calls SGX_IOC_ENCLAVE_ADD_PAGES
after enclave initialization on any memory region within the enclave
where
pages are planned to be added dynamically. This ioctl() calls EAUG to
add the
new pages with RW permissions and their vm_max_prot_bits can be set to
the
permissions found in the included SECINFO. This will support later
EACCEPTCOPY
as well as SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
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).
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
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.
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?
User space won't always have enough info to decide whether the pages to be
EAUG'd immediately. In some cases (shared libraries, JVM for example) lots
of code/data pages can be mapped but never actually touched. One
enclave/process does not know if any other more important enclave/process
would need the EPC.
It should be for kernel to make the final decision as it has overall
picture of the system EPC usage and availability.
User space can provide a hint (similar to MAP_POPULATE) to kernel that the
mmap'd area will soon be needed and kernel should EAUG as soon as it sees
fit based on current system usage. Or kernel implement some policy to
avoid #PF triggered by EACCEPT, for example, if the system has ton of free
EPC relative to the requested by mmap at the time.
BR
Haitao
