In the initial (SGX1) version of SGX, pages in an enclave need to be
created with permissions that support all usages of the pages, from the
time the enclave is initialized until it is unloaded. For example,
pages used by a JIT compiler or when code needs to otherwise be
relocated need to always have RWX permissions.
SGX2 includes two functions that can be used to modify the enclave page
permissions of regular enclave pages within an initialized enclave.
ENCLS[EMODPR] is run from the OS and used to restrict enclave page
permissions while ENCLU[EMODPE] is run from within the enclave to
extend enclave page permissions.
Enclave page permission changes need to be approached with care and
for this reason this initial support is to allow enclave page
permission changes _only_ if the new permissions are the same or
more restrictive that the permissions originally vetted at the time the
pages were added to the enclave. Support for extending enclave page
permissions beyond what was originally vetted is deferred.
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, 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. This
ioctl is used when enclave page permissions need to be restricted via
the OS as well as after enclave page permissions have been extended
from within the enclave (to ensure correct page table entries are
generated). With this ioctl the user specifies a page range and the
permissions to be applied to all pages in the provided range. The ioctl
itself can return an error code based on failures encountered by the OS.
It is also possible for SGX specific failures to be encountered. Add a
result output parameter to communicate the SGX return code. It is
possible for the permission change request to fail on a particular
page. To support partial success the ioctl will return the number
of pages that were successfully changed.
Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/include/uapi/asm/sgx.h | 20 +++
arch/x86/kernel/cpu/sgx/encl.c | 4 +-
arch/x86/kernel/cpu/sgx/encl.h | 3 +
arch/x86/kernel/cpu/sgx/ioctl.c | 235 ++++++++++++++++++++++++++++++++
4 files changed, 260 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index f4b81587e90b..24bebc31e336 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -29,6 +29,8 @@ enum sgx_page_flags {
_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_provision)
#define SGX_IOC_VEPC_REMOVE_ALL \
_IO(SGX_MAGIC, 0x04)
+#define SGX_IOC_PAGE_MODP \
+ _IOWR(SGX_MAGIC, 0x05, struct sgx_page_modp)
/**
* struct sgx_enclave_create - parameter structure for the
@@ -76,6 +78,24 @@ struct sgx_enclave_provision {
__u64 fd;
};
+/**
+ * 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;
+};
+
struct sgx_enclave_run;
/**
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index ba39186d5a28..03c4d7e00b44 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -90,8 +90,8 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page,
return epc_page;
}
-static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
- unsigned long addr)
+struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
+ unsigned long addr)
{
struct sgx_epc_page *epc_page;
struct sgx_encl_page *entry;
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index cb9f16d457ac..848a28d28d3d 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -120,4 +120,7 @@ void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
bool sgx_va_page_full(struct sgx_va_page *va_page);
void sgx_encl_free_epc_page(struct sgx_epc_page *page);
+struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
+ unsigned long addr);
+
#endif /* _X86_ENCL_H */
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 491d2700a54d..5dddb3c9f742 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -682,6 +682,238 @@ static long sgx_ioc_enclave_provision(struct sgx_encl *encl, void __user *arg)
return sgx_set_attribute(&encl->attributes_mask, params.fd);
}
+/**
+ * sgx_page_modp - Align enclave (EPCM) and OS (PTE) view of page permission
+ * @encl: Enclave to which the pages belong.
+ * @modp: Checked parameters from user on which pages need modifying
+ * and their new permissions.
+ *
+ * SGX2 distinguishes between extending and restricting the enclave page
+ * permissions maintained by the hardware (EPCM permissions) of pages
+ * belonging to an initialized enclave (after SGX_IOC_ENCLAVE_INIT).
+ *
+ * EPCM permissions can be extended anytime directly from the enclave with
+ * no visibility from the OS. This is accomplished with ENCLU[EMODPE]
+ * run from within enclave. Accessing pages with the new, extended,
+ * permissions requires the OS to update the PTE to handle the subsequent
+ * #PF correctly.
+ *
+ * EPCM permissions cannot be restricted from within the enclave, the enclave
+ * requires the OS to run the privileged level 0 instructions ENCLS[EMODPR]
+ * and ENCLS[ETRACK] to achieve this.
+ *
+ * Since OS does not have insight into enclave's ENCLU[EMODPE] calls all
+ * EPCM permission changes are treated as restricting of (EPCM) permissions.
+ * Page table entries are cleared to ensure that the fault handler installs
+ * new entries with correct permissions.
+ *
+ * Return:
+ * - 0: Success.
+ * - -errno: Otherwise.
+ */
+static long sgx_page_modp(struct sgx_encl *encl, struct sgx_page_modp *modp)
+{
+ unsigned long vm_prot, run_prot_restore;
+ struct sgx_encl_page *entry;
+ struct sgx_secinfo secinfo;
+ unsigned long addr;
+ u64 secinfo_perm;
+ unsigned long c;
+ void *epc_virt;
+ int ret;
+
+ secinfo_perm = modp->prot & SGX_SECINFO_PERMISSION_MASK;
+
+ if ((secinfo_perm & SGX_SECINFO_W) && !(secinfo_perm & SGX_SECINFO_R))
+ return -EINVAL;
+
+ memset(&secinfo, 0, sizeof(secinfo));
+
+ secinfo.flags = secinfo_perm;
+
+ vm_prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ) |
+ _calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) |
+ _calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC);
+ vm_prot = calc_vm_prot_bits(vm_prot, 0);
+
+ for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
+ addr = encl->base + modp->offset + c;
+
+ mutex_lock(&encl->lock);
+
+ entry = sgx_encl_load_page(encl, addr);
+ if (IS_ERR(entry)) {
+ ret = PTR_ERR(entry) == -EBUSY ? -EAGAIN : -EFAULT;
+ goto out_unlock;
+ }
+
+ /*
+ * Changing EPCM permissions is only supported on regular
+ * SGX pages. Attempting this change on other pages will
+ * result in #PF.
+ */
+ if (entry->type != SGX_PAGE_TYPE_REG) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ /*
+ * Do not verify if current runtime protection bits are what
+ * is being requested. The enclave may have done some
+ * permission extending calls without letting OS know and
+ * thus permission restriction may still be needed even if
+ * from OS perspective the permissions are unchanged.
+ */
+
+ /* Do not exceed permissions that have been vetted. */
+ if ((entry->vm_max_prot_bits & vm_prot) != vm_prot) {
+ ret = -EPERM;
+ goto out_unlock;
+ }
+
+ /* Make sure page stays around while releasing mutex. */
+ if (sgx_unmark_page_reclaimable(entry->epc_page)) {
+ ret = -EAGAIN;
+ goto out_unlock;
+ }
+
+ /*
+ * Change runtime protection before zapping PTEs to ensure
+ * any new #PF uses new permissions. EPCM permissions not
+ * changed yet.
+ */
+ run_prot_restore = entry->vm_run_prot_bits;
+ entry->vm_run_prot_bits = vm_prot;
+
+ mutex_unlock(&encl->lock);
+ /*
+ * Do not keep encl->lock because of dependency on
+ * mmap_lock acquired in sgx_zap_enclave_ptes().
+ */
+ sgx_zap_enclave_ptes(encl, addr);
+
+ mutex_lock(&encl->lock);
+
+ /* Change EPCM permissions. */
+ epc_virt = sgx_get_epc_virt_addr(entry->epc_page);
+ ret = __emodpr(&secinfo, epc_virt);
+ if (encls_faulted(ret)) {
+ /*
+ * All possible faults should be avoidable:
+ * parameters have been checked, will only change
+ * permissions of a regular page, and no concurrent
+ * SGX1/SGX2 ENCLS instructions since these
+ * are protected with mutex.
+ */
+ pr_err_once("EMODPR encountered exception %d\n",
+ ENCLS_TRAPNR(ret));
+ ret = -EFAULT;
+ goto out_prot_restore;
+ }
+ if (encls_failed(ret)) {
+ modp->result = ret;
+ ret = -EFAULT;
+ goto out_prot_restore;
+ }
+
+ epc_virt = sgx_get_epc_virt_addr(encl->secs.epc_page);
+ ret = __etrack(epc_virt);
+ if (ret) {
+ /*
+ * ETRACK only fails when there is an OS issue. For
+ * example, two consecutive ETRACK was sent without
+ * completed IPI between.
+ */
+ pr_err_once("ETRACK returned %d (0x%x)", ret, ret);
+ /*
+ * Send IPIs to kick CPUs out of the enclave and
+ * try ETRACK again.
+ */
+ on_each_cpu_mask(sgx_encl_cpumask(encl),
+ sgx_ipi_cb, NULL, 1);
+ ret = __etrack(epc_virt);
+ if (ret) {
+ pr_err_once("ETRACK repeat returned %d (0x%x)",
+ ret, ret);
+ ret = -EFAULT;
+ goto out_reclaim;
+ }
+ }
+ on_each_cpu_mask(sgx_encl_cpumask(encl), sgx_ipi_cb, NULL, 1);
+
+ sgx_mark_page_reclaimable(entry->epc_page);
+ mutex_unlock(&encl->lock);
+ }
+
+ ret = 0;
+ goto out;
+
+out_prot_restore:
+ entry->vm_run_prot_bits = run_prot_restore;
+out_reclaim:
+ sgx_mark_page_reclaimable(entry->epc_page);
+out_unlock:
+ mutex_unlock(&encl->lock);
+out:
+ modp->count = c;
+
+ return ret;
+}
+
+/**
+ * sgx_ioc_page_modp() - handler for %SGX_IOC_PAGE_MODP
+ * @encl: an enclave pointer
+ * @arg: userspace pointer to a &struct sgx_page_modp instance
+ *
+ * Return:
+ * - 0: Success
+ * - -errno: Otherwise
+ */
+static long sgx_ioc_page_modp(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_page_modp params;
+ long ret;
+
+ /*
+ * Ensure that there is a change this could succeed: (1) SGX2
+ * is required, and (2) only pages in an initialized enclave could
+ * be modified.
+ */
+ if (!(cpu_feature_enabled(X86_FEATURE_SGX2)))
+ return -ENODEV;
+
+ if (!test_bit(SGX_ENCL_INITIALIZED, &encl->flags))
+ return -EINVAL;
+
+ /*
+ * Obtain parameters from user and perform sanity checks.
+ */
+ if (copy_from_user(¶ms, arg, sizeof(params)))
+ return -EFAULT;
+
+ if (!IS_ALIGNED(params.offset, PAGE_SIZE))
+ return -EINVAL;
+
+ if (!params.length || params.length & (PAGE_SIZE - 1))
+ return -EINVAL;
+
+ if (params.offset + params.length - PAGE_SIZE >= encl->size)
+ return -EINVAL;
+
+ if (params.prot & ~SGX_SECINFO_PERMISSION_MASK)
+ return -EINVAL;
+
+ if (params.result || params.count)
+ return -EINVAL;
+
+ ret = sgx_page_modp(encl, ¶ms);
+
+ if (copy_to_user(arg, ¶ms, sizeof(params)))
+ return -EFAULT;
+
+ return ret;
+}
+
long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
{
struct sgx_encl *encl = filep->private_data;
@@ -703,6 +935,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
case SGX_IOC_ENCLAVE_PROVISION:
ret = sgx_ioc_enclave_provision(encl, (void __user *)arg);
break;
+ case SGX_IOC_PAGE_MODP:
+ ret = sgx_ioc_page_modp(encl, (void __user *)arg);
+ break;
default:
ret = -ENOIOCTLCMD;
break;
--
2.25.1
On 12/1/21 11:23 AM, Reinette Chatre wrote:
> + * EPCM permissions can be extended anytime directly from the enclave with
> + * no visibility from the OS. This is accomplished with ENCLU[EMODPE]
> + * run from within enclave. Accessing pages with the new, extended,
> + * permissions requires the OS to update the PTE to handle the subsequent
> + * #PF correctly.
Hi Reinette,
I really dislike the Intel nomenclature here. I know the Intel docs are
all written around permission "extension", but I find it ambiguous.
I've been looking at these instructions literally for years now and
permission extension to me can mean either:
1. The set of things you can do is extended
2. The set of things you can *NOT* do is extended
I much rather prefer nomenclature like:
EPCM permissions can be relaxed anytime directly from the
enclave with no visibility from the OS. This is accomplished
with ENCLU[EMODPE] run from within enclave. Accessing pages with
the new, relaxed permissions requires the OS to update the PTE
to handle the subsequent correctly.
"Relax" is less ambiguous. Relaxing a restriction and relaxing
permissions both mean doing things less strictly. Extending
restrictions and extending what is allowed are opposites.
Maybe it's just me and I need to get this through my thick skull at some
point. But, I do think it's OK to improve on the architecture names for
things when they go into the kernel. The XSAVE XSTATE_BV->xfeatures
rename comes to mind.
Anyway, I'd appreciate if you could keep this in mind and consider
changing it if a future revision is needed if you believe it is more clear.
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()".
> 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 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?
..
> + 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))
...
> + 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.
On 12/1/21 11:23 AM, Reinette Chatre wrote:
> Enclave page permission changes need to be approached with care and
> for this reason this initial support is to allow enclave page
> permission changes _only_ if the new permissions are the same or
> more restrictive that the permissions originally vetted at the time the
> pages were added to the enclave. Support for extending enclave page
> permissions beyond what was originally vetted is deferred.
It's probably worth adding a few examples here:
* RWX => RW => RX => RW => R => RWX
* RW => R => RW
* RX => R => RX
Hi Dave,
On 12/2/2021 3:48 PM, Dave Hansen wrote:
> On 12/1/21 11:23 AM, Reinette Chatre wrote:
>> + * EPCM permissions can be extended anytime directly from the enclave with
>> + * no visibility from the OS. This is accomplished with ENCLU[EMODPE]
>> + * run from within enclave. Accessing pages with the new, extended,
>> + * permissions requires the OS to update the PTE to handle the subsequent
>> + * #PF correctly.
>
> Hi Reinette,
>
> I really dislike the Intel nomenclature here. I know the Intel docs are
> all written around permission "extension", but I find it ambiguous.
>
> I've been looking at these instructions literally for years now and
> permission extension to me can mean either:
> 1. The set of things you can do is extended
> 2. The set of things you can *NOT* do is extended
>
> I much rather prefer nomenclature like:
>
> EPCM permissions can be relaxed anytime directly from the
> enclave with no visibility from the OS. This is accomplished
> with ENCLU[EMODPE] run from within enclave. Accessing pages with
> the new, relaxed permissions requires the OS to update the PTE
> to handle the subsequent correctly.
>
> "Relax" is less ambiguous. Relaxing a restriction and relaxing
> permissions both mean doing things less strictly. Extending
> restrictions and extending what is allowed are opposites.
Very good point.
> Maybe it's just me and I need to get this through my thick skull at some
> point. But, I do think it's OK to improve on the architecture names for
> things when they go into the kernel. The XSAVE XSTATE_BV->xfeatures
> rename comes to mind.
>
> Anyway, I'd appreciate if you could keep this in mind and consider
> changing it if a future revision is needed if you believe it is more clear.
>
Will do. I see that there is opportunity to use this terminology in my
reply to your other message in response to this patch. I'll do so and we
can then further judge how it sounds.
Reinette
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
Hi Dave,
On 12/3/2021 10:14 AM, Dave Hansen wrote:
> On 12/1/21 11:23 AM, Reinette Chatre wrote:
>> Enclave page permission changes need to be approached with care and
>> for this reason this initial support is to allow enclave page
>> permission changes _only_ if the new permissions are the same or
>> more restrictive that the permissions originally vetted at the time the
>> pages were added to the enclave. Support for extending enclave page
>> permissions beyond what was originally vetted is deferred.
>
> It's probably worth adding a few examples here:
>
> * RWX => RW => RX => RW => R => RWX
> * RW => R => RW
> * RX => R => RX
>
Indeed - that would make the implications of this change clear.
Will do. Thank you very much.
Reinette
On 12/1/21 11:23, Reinette Chatre wrote:
> In the initial (SGX1) version of SGX, pages in an enclave need to be
> created with permissions that support all usages of the pages, from the
> time the enclave is initialized until it is unloaded. For example,
> pages used by a JIT compiler or when code needs to otherwise be
> relocated need to always have RWX permissions.
>
> SGX2 includes two functions that can be used to modify the enclave page
> permissions of regular enclave pages within an initialized enclave.
> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
> permissions while ENCLU[EMODPE] is run from within the enclave to
> extend enclave page permissions.
>
> Enclave page permission changes need to be approached with care and
> for this reason this initial support is to allow enclave page
> permission changes _only_ if the new permissions are the same or
> more restrictive that the permissions originally vetted at the time the
> pages were added to the enclave. Support for extending enclave page
> permissions beyond what was originally vetted is deferred.
>
I may well be missing something, but off the top of my head, literally
the only reason that EMODPR needs CPL0 (i.e. ENCLS) is that it requires
a TLB flush IPI to take effect. (Score one for AMD for being having
superior hardware in this regard.)
Given that, I don't see any reason for the EMODPR operation to be
treated as security sensitive -- it just needs to be implemented
correctly. I don't even see why the host should (or even can!) do any
useful tracking of the EPCM state.
(But I am confused about one thing: to the extent an enclave actually
needs EMODPR, is there anything in the hardware or anything that the
enclave can do short of actually poking the page from all threads and
confirming that a fault occurs to make sure the OS actually flushed the
TLB? ISTM a malicious host could attack an enclave by omitting the TLB
flush and then exploiting an enclave but that would have been mitigated
if the flush occurred.)
--Andy
Hi Andy,
On 12/3/2021 11:38 AM, Andy Lutomirski wrote:
> On 12/1/21 11:23, Reinette Chatre wrote:
>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>> created with permissions that support all usages of the pages, from the
>> time the enclave is initialized until it is unloaded. For example,
>> pages used by a JIT compiler or when code needs to otherwise be
>> relocated need to always have RWX permissions.
>>
>> SGX2 includes two functions that can be used to modify the enclave page
>> permissions of regular enclave pages within an initialized enclave.
>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>> permissions while ENCLU[EMODPE] is run from within the enclave to
>> extend enclave page permissions.
>>
>> Enclave page permission changes need to be approached with care and
>> for this reason this initial support is to allow enclave page
>> permission changes _only_ if the new permissions are the same or
>> more restrictive that the permissions originally vetted at the time the
>> pages were added to the enclave. Support for extending enclave page
>> permissions beyond what was originally vetted is deferred.
>>
>
> I may well be missing something, but off the top of my head, literally
> the only reason that EMODPR needs CPL0 (i.e. ENCLS) is that it requires
> a TLB flush IPI to take effect. (Score one for AMD for being having
> superior hardware in this regard.)
My understanding also is that it is the need for TLB flush that require
the privilege but I am trying to get more information here.
>
> Given that, I don't see any reason for the EMODPR operation to be
> treated as security sensitive -- it just needs to be implemented
> correctly. I don't even see why the host should (or even can!) do any
> useful tracking of the EPCM state.
The OS needs to know the EPCM permissions to be able to install the
appropriate PTEs. If the enclave chooses to change the enclave page
permissions from within the enclave then user space needs to let the OS
know via the SGX_IOC_PAGE_MODP ioctl to ensure that the OS can install
correct PTEs in support of the permission change.
> (But I am confused about one thing: to the extent an enclave actually
> needs EMODPR, is there anything in the hardware or anything that the
> enclave can do short of actually poking the page from all threads and
> confirming that a fault occurs to make sure the OS actually flushed the
> TLB? ISTM a malicious host could attack an enclave by omitting the TLB
> flush and then exploiting an enclave but that would have been mitigated
> if the flush occurred.)
When enclave page permissions are restricted it requires the enclave to
accept the new permissions from within the enclave by running
ENCLU[EACCEPT]. This instruction requires that (it will fail otherwise)
the OS completed an ENCLS[ETRACK] on the affected page - essentially
ENCLU[EACCEPT] can only succeed if no cached linear-to-physical address
mappings are present. The ETRACK flow is elaborate and I attempted to
document it in patch 06/25. Essentially, SGX hardware flushes all cached
linear-to-physical mappings when an enclave is exited and with ETRACK it
can be ensured that all threads that were in an enclave at the time the
tracking started (in this case after ENCLS[EMODPR]), have exited.
Reinette
On 12/3/21 14:34, Reinette Chatre wrote:
> Hi Andy,
>
> On 12/3/2021 11:38 AM, Andy Lutomirski wrote:
>> On 12/1/21 11:23, Reinette Chatre wrote:
>>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>>> created with permissions that support all usages of the pages, from the
>>> time the enclave is initialized until it is unloaded. For example,
>>> pages used by a JIT compiler or when code needs to otherwise be
>>> relocated need to always have RWX permissions.
>>>
>>> SGX2 includes two functions that can be used to modify the enclave page
>>> permissions of regular enclave pages within an initialized enclave.
>>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>>> permissions while ENCLU[EMODPE] is run from within the enclave to
>>> extend enclave page permissions.
>>>
>>> Enclave page permission changes need to be approached with care and
>>> for this reason this initial support is to allow enclave page
>>> permission changes _only_ if the new permissions are the same or
>>> more restrictive that the permissions originally vetted at the time the
>>> pages were added to the enclave. Support for extending enclave page
>>> permissions beyond what was originally vetted is deferred.
>>>
>>
>> I may well be missing something, but off the top of my head, literally
>> the only reason that EMODPR needs CPL0 (i.e. ENCLS) is that it
>> requires a TLB flush IPI to take effect. (Score one for AMD for being
>> having superior hardware in this regard.)
>
> My understanding also is that it is the need for TLB flush that require
> the privilege but I am trying to get more information here.
>
>>
>> Given that, I don't see any reason for the EMODPR operation to be
>> treated as security sensitive -- it just needs to be implemented
>> correctly. I don't even see why the host should (or even can!) do any
>> useful tracking of the EPCM state.
>
> The OS needs to know the EPCM permissions to be able to install the
> appropriate PTEs. If the enclave chooses to change the enclave page
> permissions from within the enclave then user space needs to let the OS
> know via the SGX_IOC_PAGE_MODP ioctl to ensure that the OS can install
> correct PTEs in support of the permission change.
>
>
>> (But I am confused about one thing: to the extent an enclave actually
>> needs EMODPR, is there anything in the hardware or anything that the
>> enclave can do short of actually poking the page from all threads and
>> confirming that a fault occurs to make sure the OS actually flushed
>> the TLB? ISTM a malicious host could attack an enclave by omitting
>> the TLB flush and then exploiting an enclave but that would have been
>> mitigated if the flush occurred.)
>
> When enclave page permissions are restricted it requires the enclave to
> accept the new permissions from within the enclave by running
> ENCLU[EACCEPT]. This instruction requires that (it will fail otherwise)
> the OS completed an ENCLS[ETRACK] on the affected page - essentially
> ENCLU[EACCEPT] can only succeed if no cached linear-to-physical address
> mappings are present. The ETRACK flow is elaborate and I attempted to
> document it in patch 06/25. Essentially, SGX hardware flushes all cached
> linear-to-physical mappings when an enclave is exited and with ETRACK it
> can be ensured that all threads that were in an enclave at the time the
> tracking started (in this case after ENCLS[EMODPR]), have exited.
>
Does the enclave do something before asking for the ioctl to put the
page in a state where the tracking is armed? I read the SDM, but I
probably read the wrong part of the SDM, and I may have missed this.
Hi Andy,
On 12/3/2021 4:42 PM, Andy Lutomirski wrote:
> On 12/3/21 14:34, Reinette Chatre wrote:
>> On 12/3/2021 11:38 AM, Andy Lutomirski wrote:
>>> On 12/1/21 11:23, Reinette Chatre wrote:
>>>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>>>> created with permissions that support all usages of the pages, from the
>>>> time the enclave is initialized until it is unloaded. For example,
>>>> pages used by a JIT compiler or when code needs to otherwise be
>>>> relocated need to always have RWX permissions.
>>>>
>>>> SGX2 includes two functions that can be used to modify the enclave page
>>>> permissions of regular enclave pages within an initialized enclave.
>>>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>>>> permissions while ENCLU[EMODPE] is run from within the enclave to
>>>> extend enclave page permissions.
>>>>
>>>> Enclave page permission changes need to be approached with care and
>>>> for this reason this initial support is to allow enclave page
>>>> permission changes _only_ if the new permissions are the same or
>>>> more restrictive that the permissions originally vetted at the time the
>>>> pages were added to the enclave. Support for extending enclave page
>>>> permissions beyond what was originally vetted is deferred.
>>>>
>>>
>>> I may well be missing something, but off the top of my head,
>>> literally the only reason that EMODPR needs CPL0 (i.e. ENCLS) is that
>>> it requires a TLB flush IPI to take effect. (Score one for AMD for
>>> being having superior hardware in this regard.)
>>
>> My understanding also is that it is the need for TLB flush that
>> require the privilege but I am trying to get more information here.
>>
>>>
>>> Given that, I don't see any reason for the EMODPR operation to be
>>> treated as security sensitive -- it just needs to be implemented
>>> correctly. I don't even see why the host should (or even can!) do
>>> any useful tracking of the EPCM state.
>>
>> The OS needs to know the EPCM permissions to be able to install the
>> appropriate PTEs. If the enclave chooses to change the enclave page
>> permissions from within the enclave then user space needs to let the
>> OS know via the SGX_IOC_PAGE_MODP ioctl to ensure that the OS can
>> install correct PTEs in support of the permission change.
>>
>>
>>> (But I am confused about one thing: to the extent an enclave actually
>>> needs EMODPR, is there anything in the hardware or anything that the
>>> enclave can do short of actually poking the page from all threads and
>>> confirming that a fault occurs to make sure the OS actually flushed
>>> the TLB? ISTM a malicious host could attack an enclave by omitting
>>> the TLB flush and then exploiting an enclave but that would have been
>>> mitigated if the flush occurred.)
>>
>> When enclave page permissions are restricted it requires the enclave
>> to accept the new permissions from within the enclave by running
>> ENCLU[EACCEPT]. This instruction requires that (it will fail
>> otherwise) the OS completed an ENCLS[ETRACK] on the affected page -
>> essentially ENCLU[EACCEPT] can only succeed if no cached
>> linear-to-physical address mappings are present. The ETRACK flow is
>> elaborate and I attempted to document it in patch 06/25. Essentially,
>> SGX hardware flushes all cached linear-to-physical mappings when an
>> enclave is exited and with ETRACK it can be ensured that all threads
>> that were in an enclave at the time the tracking started (in this case
>> after ENCLS[EMODPR]), have exited.
>>
>
> Does the enclave do something before asking for the ioctl to put the
> page in a state where the tracking is armed? I read the SDM, but I
> probably read the wrong part of the SDM, and I may have missed this.
No, when restricting permissions the enclave does not do anything
special to the page before calling the ioctl.
The (non enclave) userspace calls the SGX_IOC_PAGE_MODP ioctl that will
call ENCLS[EMODPR] to restrict the permissions as well as the
ENCLS[ETRACK] to start the tracking before sending all CPUs that may be
accessing the enclave an IPI. The enclave then runs ENCLU[EACCEPT] to
accept the permission changes and this would fail if the host attempted
to omit the TLB flush.
You can see an example of EPCM permission changes from user space
perspective in the form of a selftest found in the patch that follows
this one.
Reinette
On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
> In the initial (SGX1) version of SGX, pages in an enclave need to be
> created with permissions that support all usages of the pages, from the
> time the enclave is initialized until it is unloaded. For example,
> pages used by a JIT compiler or when code needs to otherwise be
> relocated need to always have RWX permissions.
>
> SGX2 includes two functions that can be used to modify the enclave page
> permissions of regular enclave pages within an initialized enclave.
> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
> permissions while ENCLU[EMODPE] is run from within the enclave to
> extend enclave page permissions.
>
> Enclave page permission changes need to be approached with care and
> for this reason this initial support is to allow enclave page
> permission changes _only_ if the new permissions are the same or
> more restrictive that the permissions originally vetted at the time the
> pages were added to the enclave. Support for extending enclave page
> permissions beyond what was originally vetted is deferred.
This paragraph is out-of-scope for a commit message. You could have
this in the cover letter but not here. I would just remove it.
> 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, SGX_IOC_PAGE_MODP, to
SGX_IOC_PAGE_MODP does not match the naming convetion of these:
* SGX_IOC_ENCLAVE_CREATE
* SGX_IOC_ENCLAVE_ADD_PAGES
* SGX_IOC_ENCLAVE_INIT
A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
do harm to be a more verbose.
/Jarkko
On 12/4/21 3:08 PM, Jarkko Sakkinen wrote:
>> Enclave page permission changes need to be approached with care and
>> for this reason this initial support is to allow enclave page
>> permission changes _only_ if the new permissions are the same or
>> more restrictive that the permissions originally vetted at the time the
>> pages were added to the enclave. Support for extending enclave page
>> permissions beyond what was originally vetted is deferred.
> This paragraph is out-of-scope for a commit message. You could have
> this in the cover letter but not here. I would just remove it.
This does convey valuable information, though. It tells the reader that
this is a sub-optimal implementation. It also acknowledges that there
is further work to do. Maybe saying that it is "deferred" is not quite
the verbiage I would use, but the concept is fine.
Hi Jarkko,
On 12/4/2021 3:08 PM, Jarkko Sakkinen wrote:
> On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>> created with permissions that support all usages of the pages, from the
>> time the enclave is initialized until it is unloaded. For example,
>> pages used by a JIT compiler or when code needs to otherwise be
>> relocated need to always have RWX permissions.
>>
>> SGX2 includes two functions that can be used to modify the enclave page
>> permissions of regular enclave pages within an initialized enclave.
>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>> permissions while ENCLU[EMODPE] is run from within the enclave to
>> extend enclave page permissions.
>>
>> Enclave page permission changes need to be approached with care and
>> for this reason this initial support is to allow enclave page
>> permission changes _only_ if the new permissions are the same or
>> more restrictive that the permissions originally vetted at the time the
>> pages were added to the enclave. Support for extending enclave page
>> permissions beyond what was originally vetted is deferred.
>
> This paragraph is out-of-scope for a commit message. You could have
> this in the cover letter but not here. I would just remove it.
I think this is essential information that is mentioned in the cover
letter _and_ in this changelog. I will follow Dave's guidance and avoid
"deferred" by just removing that last sentence.
>
>> 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, SGX_IOC_PAGE_MODP, to
>
> SGX_IOC_PAGE_MODP does not match the naming convetion of these:
>
> * SGX_IOC_ENCLAVE_CREATE
> * SGX_IOC_ENCLAVE_ADD_PAGES
> * SGX_IOC_ENCLAVE_INIT
ah - my understanding was that the SGX_IOC_ENCLAVE prefix related to
operations related to the entire enclave and thus I introduced the
prefix SGX_IOC_PAGE to relate to operations on pages within an enclave.
>
> A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
> do harm to be a more verbose.
Will do. I see later you propose SGX_IOC_ENCLAVE_MODIFY_TYPE - would you
like them to be consistent wrt MOD/MODIFY?
Reinette
On Mon, 2021-12-06 at 12:19 -0800, Dave Hansen wrote:
> On 12/4/21 3:08 PM, Jarkko Sakkinen wrote:
> > > Enclave page permission changes need to be approached with care and
> > > for this reason this initial support is to allow enclave page
> > > permission changes _only_ if the new permissions are the same or
> > > more restrictive that the permissions originally vetted at the time the
> > > pages were added to the enclave. Support for extending enclave page
> > > permissions beyond what was originally vetted is deferred.
> > This paragraph is out-of-scope for a commit message. You could have
> > this in the cover letter but not here. I would just remove it.
>
> This does convey valuable information, though. It tells the reader that
> this is a sub-optimal implementation. It also acknowledges that there
> is further work to do. Maybe saying that it is "deferred" is not quite
> the verbiage I would use, but the concept is fine.
BTW, should we consistently speak about protection bits instead of
permissions?
/Jarkko
On Mon, 2021-12-06 at 13:42 -0800, Reinette Chatre wrote:
> Hi Jarkko,
>
> On 12/4/2021 3:08 PM, Jarkko Sakkinen wrote:
> > On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
> > > In the initial (SGX1) version of SGX, pages in an enclave need to be
> > > created with permissions that support all usages of the pages, from the
> > > time the enclave is initialized until it is unloaded. For example,
> > > pages used by a JIT compiler or when code needs to otherwise be
> > > relocated need to always have RWX permissions.
> > >
> > > SGX2 includes two functions that can be used to modify the enclave page
> > > permissions of regular enclave pages within an initialized enclave.
> > > ENCLS[EMODPR] is run from the OS and used to restrict enclave page
> > > permissions while ENCLU[EMODPE] is run from within the enclave to
> > > extend enclave page permissions.
> > >
> > > Enclave page permission changes need to be approached with care and
> > > for this reason this initial support is to allow enclave page
> > > permission changes _only_ if the new permissions are the same or
> > > more restrictive that the permissions originally vetted at the time the
> > > pages were added to the enclave. Support for extending enclave page
> > > permissions beyond what was originally vetted is deferred.
> >
> > This paragraph is out-of-scope for a commit message. You could have
> > this in the cover letter but not here. I would just remove it.
>
> I think this is essential information that is mentioned in the cover
> letter _and_ in this changelog. I will follow Dave's guidance and avoid
> "deferred" by just removing that last sentence.
>
> >
> > > 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, SGX_IOC_PAGE_MODP, to
> >
> > SGX_IOC_PAGE_MODP does not match the naming convetion of these:
> >
> > * SGX_IOC_ENCLAVE_CREATE
> > * SGX_IOC_ENCLAVE_ADD_PAGES
> > * SGX_IOC_ENCLAVE_INIT
>
> ah - my understanding was that the SGX_IOC_ENCLAVE prefix related to
> operations related to the entire enclave and thus I introduced the
> prefix SGX_IOC_PAGE to relate to operations on pages within an enclave.
SGX_IOC_ENCLAVE_ADD_PAGES is also operation working on pages within an
enclave.
Also, to be aligned with SGX_IOC_ENCLAVE_ADD_PAGES, the new operations
should also take secinfo as input.
>
> >
> > A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
> > do harm to be a more verbose.
>
> Will do. I see later you propose SGX_IOC_ENCLAVE_MODIFY_TYPE - would you
> like them to be consistent wrt MOD/MODIFY?
I would considering introducing just one new ioctl:
SGX_IOC_ENCLAVE_MODIFY_PAGES
and choose either operations based on e.g. a flag
(see flags field SGX_IOC_ENCLAVE_ADD_PAGES).
> Reinette
/Jarkko
Hi Jarkko,
On 12/10/2021 11:57 PM, Jarkko Sakkinen wrote:
> On Mon, 2021-12-06 at 13:42 -0800, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 12/4/2021 3:08 PM, Jarkko Sakkinen wrote:
>>> On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
>>>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>>>> created with permissions that support all usages of the pages, from the
>>>> time the enclave is initialized until it is unloaded. For example,
>>>> pages used by a JIT compiler or when code needs to otherwise be
>>>> relocated need to always have RWX permissions.
>>>>
>>>> SGX2 includes two functions that can be used to modify the enclave page
>>>> permissions of regular enclave pages within an initialized enclave.
>>>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>>>> permissions while ENCLU[EMODPE] is run from within the enclave to
>>>> extend enclave page permissions.
>>>>
>>>> Enclave page permission changes need to be approached with care and
>>>> for this reason this initial support is to allow enclave page
>>>> permission changes _only_ if the new permissions are the same or
>>>> more restrictive that the permissions originally vetted at the time the
>>>> pages were added to the enclave. Support for extending enclave page
>>>> permissions beyond what was originally vetted is deferred.
>>>
>>> This paragraph is out-of-scope for a commit message. You could have
>>> this in the cover letter but not here. I would just remove it.
>>
>> I think this is essential information that is mentioned in the cover
>> letter _and_ in this changelog. I will follow Dave's guidance and avoid
>> "deferred" by just removing that last sentence.
>>
>>>
>>>> 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, SGX_IOC_PAGE_MODP, to
>>>
>>> SGX_IOC_PAGE_MODP does not match the naming convetion of these:
>>>
>>> * SGX_IOC_ENCLAVE_CREATE
>>> * SGX_IOC_ENCLAVE_ADD_PAGES
>>> * SGX_IOC_ENCLAVE_INIT
>>
>> ah - my understanding was that the SGX_IOC_ENCLAVE prefix related to
>> operations related to the entire enclave and thus I introduced the
>> prefix SGX_IOC_PAGE to relate to operations on pages within an enclave.
>
> SGX_IOC_ENCLAVE_ADD_PAGES is also operation working on pages within an
> enclave.
>
> Also, to be aligned with SGX_IOC_ENCLAVE_ADD_PAGES, the new operations
> should also take secinfo as input.
ok, will do.
>
>>
>>>
>>> A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
>>> do harm to be a more verbose.
>>
>> Will do. I see later you propose SGX_IOC_ENCLAVE_MODIFY_TYPE - would you
>> like them to be consistent wrt MOD/MODIFY?
>
> I would considering introducing just one new ioctl:
>
> SGX_IOC_ENCLAVE_MODIFY_PAGES
>
> and choose either operations based on e.g. a flag
> (see flags field SGX_IOC_ENCLAVE_ADD_PAGES).
>
There seems to be different opinion about the single ioctl() as
per:https://lore.kernel.org/lkml/[email protected]/
I thus plan to proceed with the two ioctls, both taking secinfo as
input. Would that be ok with you?
Reinette
On Mon, Dec 13, 2021 at 02:12:44PM -0800, Reinette Chatre wrote:
> Hi Jarkko,
>
> On 12/10/2021 11:57 PM, Jarkko Sakkinen wrote:
> > On Mon, 2021-12-06 at 13:42 -0800, Reinette Chatre wrote:
> > > Hi Jarkko,
> > >
> > > On 12/4/2021 3:08 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
> > > > > In the initial (SGX1) version of SGX, pages in an enclave need to be
> > > > > created with permissions that support all usages of the pages, from the
> > > > > time the enclave is initialized until it is unloaded. For example,
> > > > > pages used by a JIT compiler or when code needs to otherwise be
> > > > > relocated need to always have RWX permissions.
> > > > >
> > > > > SGX2 includes two functions that can be used to modify the enclave page
> > > > > permissions of regular enclave pages within an initialized enclave.
> > > > > ENCLS[EMODPR] is run from the OS and used to restrict enclave page
> > > > > permissions while ENCLU[EMODPE] is run from within the enclave to
> > > > > extend enclave page permissions.
> > > > >
> > > > > Enclave page permission changes need to be approached with care and
> > > > > for this reason this initial support is to allow enclave page
> > > > > permission changes _only_ if the new permissions are the same or
> > > > > more restrictive that the permissions originally vetted at the time the
> > > > > pages were added to the enclave. Support for extending enclave page
> > > > > permissions beyond what was originally vetted is deferred.
> > > >
> > > > This paragraph is out-of-scope for a commit message. You could have
> > > > this in the cover letter but not here. I would just remove it.
> > >
> > > I think this is essential information that is mentioned in the cover
> > > letter _and_ in this changelog. I will follow Dave's guidance and avoid
> > > "deferred" by just removing that last sentence.
> > >
> > > >
> > > > > 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, SGX_IOC_PAGE_MODP, to
> > > >
> > > > SGX_IOC_PAGE_MODP does not match the naming convetion of these:
> > > >
> > > > * SGX_IOC_ENCLAVE_CREATE
> > > > * SGX_IOC_ENCLAVE_ADD_PAGES
> > > > * SGX_IOC_ENCLAVE_INIT
> > >
> > > ah - my understanding was that the SGX_IOC_ENCLAVE prefix related to
> > > operations related to the entire enclave and thus I introduced the
> > > prefix SGX_IOC_PAGE to relate to operations on pages within an enclave.
> >
> > SGX_IOC_ENCLAVE_ADD_PAGES is also operation working on pages within an
> > enclave.
> >
> > Also, to be aligned with SGX_IOC_ENCLAVE_ADD_PAGES, the new operations
> > should also take secinfo as input.
>
> ok, will do.
>
> >
> > >
> > > >
> > > > A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
> > > > do harm to be a more verbose.
> > >
> > > Will do. I see later you propose SGX_IOC_ENCLAVE_MODIFY_TYPE - would you
> > > like them to be consistent wrt MOD/MODIFY?
> >
> > I would considering introducing just one new ioctl:
> >
> > SGX_IOC_ENCLAVE_MODIFY_PAGES
> >
> > and choose either operations based on e.g. a flag
> > (see flags field SGX_IOC_ENCLAVE_ADD_PAGES).
> >
>
> There seems to be different opinion about the single ioctl() as per:https://lore.kernel.org/lkml/[email protected]/
>
> I thus plan to proceed with the two ioctls, both taking secinfo as input.
> Would that be ok with you?
Yeah, let's continue with two ioctls for now, I agree.
/Jarkko