Hi Dave,
On 12/2/2021 4:32 PM, Dave Hansen wrote:
On 12/1/21 11:23 AM, Reinette Chatre wrote:
Whether enclave page permissions are restricted or extended it
is necessary to ensure that the page table entries and enclave page
permissions are in sync. Introduce a new ioctl,
These should be "ioctl()".
Will fix.
SGX_IOC_PAGE_MODP, to support enclave page permission changes. Since
the OS has no insight in how permissions may have been extended from
within the enclave all page permission requests are treated as
permission restrictions.
I'm trying to wrap my head around this a bit. If this is only for
restricting permissions, should we be reflecting that in the naming?
SGX_IOC_PAGE_RESTRICT_PERM, perhaps? Wouldn't that be more direct than
saying, "here's a permission change ioctl(), but it doesn't arbitrarily
change things, it treats all changes as restrictions"?
The ioctl is named from the user space perspective as opposed to the OS
perspective. While the OS treats all permission changes as permission
restrictions, user space needs to call this ioctl() to support all
enclave page permission changes:
* If the enclave page permissions are being restricted then this ioctl()
would clear the page table entries and call ENCLS[EMODPR] that would
have work to do to change the enclave page permissions.
* If the enclave page permissions are relaxed (should have been preceded
by ENCLU[EMODPE] from within the enclave) then this ioctl() would do the
same as in previous bullet (most importantly clear the page tables) but
in this case ENCLS[EMODPR] would be a no-op as you indicate below.
Since user space needs OS support for both relaxing and restriction of
permissions "SGX_IOC_PAGE_MODP" seemed appropriate.
The pseudocode for EMODPR looks like this:
(* Update EPCM permissions *)
EPCM(DS:RCX).R := EPCM(DS:RCX).R & SCRATCH_SECINFO.FLAGS.R;
EPCM(DS:RCX).W := EPCM(DS:RCX).W & SCRATCH_SECINFO.FLAGS.W;
EPCM(DS:RCX).X := EPCM(DS:RCX).X & SCRATCH_SECINFO.FLAGS.X;
so it makes total sense that it can only restrict permissions since it's
effectively:
new_hw_perm = old_hw_perm & secinfo_perm;
...
+/**
+ * struct sgx_page_modp - parameter structure for the %SGX_IOC_PAGE_MODP ioctl
+ * @offset: starting page offset (page aligned relative to enclave base
+ * address defined in SECS)
+ * @length: length of memory (multiple of the page size)
+ * @prot: new protection bits of pages in range described by @offset
+ * and @length
+ * @result: SGX result code of ENCLS[EMODPR] function
+ * @count: bytes successfully changed (multiple of page size)
+ */
+struct sgx_page_modp {
+ __u64 offset;
+ __u64 length;
+ __u64 prot;
+ __u64 result;
+ __u64 count;
+};
Could we make it more explicit that offset/length/prot are inputs and
result/count are output?
This follows the pattern of existing struct sgx_enclave_add_pages. Could
you please provide guidance or a reference of what you would like to
see? I scanned all the files in arch/x86/include/uapi/asm/* defining RW
ioctls and a few files in include/uapi/linux/* and I was not able to
notice such a custom.
Would you like to see something like a "in_"/"out_" prefix? If so, would
you like to see a preparatory patch that changes struct
sgx_enclave_add_pages also? If needed, I am not sure how to handle the
latter due to the possible user space impact.
..
+ if (!params.length || params.length & (PAGE_SIZE - 1))
+ return -EINVAL;
I find these a bit easier to read if they're:
if (!params.length || !IS_ALIGNED(params.length, PAGE_SIZE))
...
I am not sure about this. First, (I understand this is not a reason to
do things a particular way), this is re-using the vetted code from
sgx_ioc_enclave_add_pages(). Second, my understanding of IS_ALIGNED is
its use to indicate that a provided address/offset is on some boundary,
in this case it is the length field being verified (not an address or
offset) and it is required to be a multiple of the page size.
I understand that the code ends up being the same but I think that it
may be hard to parse that a length field is required to be aligned.
No objection to changing this if you prefer using IS_ALIGNED and I will
then also include a preparatory patch to change
sgx_ioc_enclave_add_pages() and make the same change in the following
patches.
Could you please let me know what you prefer?
+ if (params.offset + params.length - PAGE_SIZE >= encl->size)
+ return -EINVAL;
I hate boundary conditions. :) But, I think this would be simpler
written as:
if (params.offset + params.length > encl->size)
Please double-check me on that, though. I've gotten these kinds of
checks wrong more times than I care to admit.
I am very cautious about boundary conditions and thus preferred to
re-use the existing checks from sgx_ioc_enclave_add_pages(). Your
suggestion is much simpler though and I will use it. Would you also like
to see a preparatory patch that changes the existing check in
sgx_ioc_enclave_add_pages()?
Reinette
