* sgx_encl_may_map()
@ 2019-08-23 15:02 Jarkko Sakkinen
2019-08-23 16:04 ` sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 20:37 ` sgx_encl_may_map() Sean Christopherson
0 siblings, 2 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2019-08-23 15:02 UTC (permalink / raw)
To: sean.j.christopherson; +Cc: luto, linux-sgx
Hi
Couple of remarks that are not reasoned in the code too well:
1. Why to allow to mmap() to a range where pages have differing
permissions? I think it would be better just to require all
pages in a range to have the same permissions.
2. Why not require exact match with the PROT bits?
3. Why don't just store SECINFO flags on EADD? Doing
calc_vma_prot_bits() on fly is not expensive. Now
data in SECINFO flags is scattered to two different
fields (desc and vm_max_prot_bits). I'd just have a field
called secinfo_flags.
This came up when I noted that SGX_IOC_ENCLAVE_ADD_PAGE documentation
was not updated when this permission handling came up. I started to
do it but stumped into these questions. I also wonder why the API
documentation has not been updated...
/Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: sgx_encl_may_map()
2019-08-23 15:02 sgx_encl_may_map() Jarkko Sakkinen
@ 2019-08-23 16:04 ` Jarkko Sakkinen
2019-08-23 16:27 ` sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 20:37 ` sgx_encl_may_map() Sean Christopherson
1 sibling, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2019-08-23 16:04 UTC (permalink / raw)
To: sean.j.christopherson; +Cc: luto, linux-sgx
On Fri, Aug 23, 2019 at 06:02:48PM +0300, Jarkko Sakkinen wrote:
> 3. Why don't just store SECINFO flags on EADD? Doing
> calc_vma_prot_bits() on fly is not expensive. Now
> data in SECINFO flags is scattered to two different
> fields (desc and vm_max_prot_bits). I'd just have a field
> called secinfo_flags.
Makes sense to have that field as a cache so that we don't have to do
recalc per page in the sgx_encl_may_map() and thus makes sense to keep
TCS tagging in desc.
/Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: sgx_encl_may_map()
2019-08-23 16:04 ` sgx_encl_may_map() Jarkko Sakkinen
@ 2019-08-23 16:27 ` Jarkko Sakkinen
0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2019-08-23 16:27 UTC (permalink / raw)
To: sean.j.christopherson; +Cc: luto, linux-sgx, serge.ayoun, shay.katz-zamir
On Fri, Aug 23, 2019 at 07:04:04PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 23, 2019 at 06:02:48PM +0300, Jarkko Sakkinen wrote:
> > 3. Why don't just store SECINFO flags on EADD? Doing
> > calc_vma_prot_bits() on fly is not expensive. Now
> > data in SECINFO flags is scattered to two different
> > fields (desc and vm_max_prot_bits). I'd just have a field
> > called secinfo_flags.
>
> Makes sense to have that field as a cache so that we don't have to do
> recalc per page in the sgx_encl_may_map() and thus makes sense to keep
> TCS tagging in desc.
(1) and (2) I will remark in v22 patch review. It is better to do that
way.
I squashed my remaining fixes that I just posted to the patch set.
The final change log for v22 is:
* Alignment checks have been removed from mmap() because it does not define the
ELRANGE. VMAs only act as windows to the enclave. The semantics compare
somewhat how mmap() works with regular files.
* We now require user space addresses given to SGX_IOC_ENCLAVE_ADD_PAGE to be
page aligned so that we can pass the page directly to EADD and do not have
to do an extra copy. This was made effectively possible by removing the
worker thread for adding pages.
* The selftest build files have been refined throughout of various glitches
and work properly in a cross compilation environment such as BuildRoot.
In addition, libcalls fail the build with an assertion in the linker
script, if they end up to the enclave binary.
* CONFIG_INTEL_SGX_DRIVER has been removed because you cannot use SGX core
for anything without having the driver. This could change when KVM support
is added.
* We require zero permissions in SECINFO for TCS pages because the CPU
overwrites SECINFO flags with zero permissions and measures the page
only after that. Allowing to pass TCS with non-zero permissions would
cause mismatching measurement between the one provided in SIGSTRUCT and
the one computed by the CPU.
As I noted I can ATM run only selftest as the stress test that I'm using
is not yet working with the latets API changes.
Can you give it a shot?
/Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: sgx_encl_may_map()
2019-08-23 15:02 sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 16:04 ` sgx_encl_may_map() Jarkko Sakkinen
@ 2019-08-23 20:37 ` Sean Christopherson
2019-08-26 2:56 ` sgx_encl_may_map() Jarkko Sakkinen
1 sibling, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2019-08-23 20:37 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: luto, linux-sgx
On Fri, Aug 23, 2019 at 06:02:48PM +0300, Jarkko Sakkinen wrote:
> Hi
>
> Couple of remarks that are not reasoned in the code too well:
>
> 1. Why to allow to mmap() to a range where pages have differing
> permissions? I think it would be better just to require all
> pages in a range to have the same permissions.
You mean different vm_max_prot_bits? I don't think we should deny
mapping a mix of R and RW pages with PROT_READ, e.g. map read-only but
have the option to make select pages writable in the future.
> 2. Why not require exact match with the PROT bits?
Toggling W in particular in the PTEs likely has value, e.g. initialize
data with runtime info and then lock the range in the PTEs.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: sgx_encl_may_map()
2019-08-23 20:37 ` sgx_encl_may_map() Sean Christopherson
@ 2019-08-26 2:56 ` Jarkko Sakkinen
0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2019-08-26 2:56 UTC (permalink / raw)
To: Sean Christopherson; +Cc: luto, linux-sgx
On Fri, Aug 23, 2019 at 01:37:09PM -0700, Sean Christopherson wrote:
> On Fri, Aug 23, 2019 at 06:02:48PM +0300, Jarkko Sakkinen wrote:
> > Hi
> >
> > Couple of remarks that are not reasoned in the code too well:
> >
> > 1. Why to allow to mmap() to a range where pages have differing
> > permissions? I think it would be better just to require all
> > pages in a range to have the same permissions.
>
> You mean different vm_max_prot_bits? I don't think we should deny
> mapping a mix of R and RW pages with PROT_READ, e.g. map read-only but
> have the option to make select pages writable in the future.
>
> > 2. Why not require exact match with the PROT bits?
>
> Toggling W in particular in the PTEs likely has value, e.g. initialize
> data with runtime info and then lock the range in the PTEs.
Thanks.
This is really important to document. I'll send a kdoc update for the
ioctl.
/Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-26 2:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 15:02 sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 16:04 ` sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 16:27 ` sgx_encl_may_map() Jarkko Sakkinen
2019-08-23 20:37 ` sgx_encl_may_map() Sean Christopherson
2019-08-26 2:56 ` sgx_encl_may_map() Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).