| Message ID | 20200915112842.897265-18-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX foundations | expand |
> Subject: Re: [PATCH v38 17/24] x86/sgx: ptrace() support for the SGX driver ... x86/sgx: Add ptrace() support... subject needs a verb. On Tue, Sep 15, 2020 at 02:28:35PM +0300, Jarkko Sakkinen wrote: > Add VMA callbacks for ptrace() that can be used with debug enclaves. > With debug enclaves data can be read and write the memory word at a time I think you wanna say here "... data can be read and/or written a memory word at a time by using..." > by using ENCLS(EDBGRD) and ENCLS(EDBGWR) leaf instructions. > > Acked-by: Jethro Beekman <jethro@fortanix.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 87 ++++++++++++++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 11ec2df59b54..7f8df2c8ef35 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -333,10 +333,97 @@ static int sgx_vma_mprotect(struct vm_area_struct *vma, > return mprotect_fixup(vma, pprev, start, end, newflags); > } > > +static int sgx_edbgrd(struct sgx_encl *encl, struct sgx_encl_page *page, > + unsigned long addr, void *data) > +{ > + unsigned long offset = addr & ~PAGE_MASK; > + int ret; > + > + > + ret = __edbgrd(sgx_get_epc_addr(page->epc_page) + offset, data); > + if (ret) > + return -EIO; > + > + return 0; > +} > + > +static int sgx_edbgwr(struct sgx_encl *encl, struct sgx_encl_page *page, > + unsigned long addr, void *data) > +{ > + unsigned long offset = addr & ~PAGE_MASK; > + int ret; > + > + ret = __edbgwr(sgx_get_epc_addr(page->epc_page) + offset, data); > + if (ret) > + return -EIO; > + > + return 0; > +} I know those are supposed to correspond to the ENCLS leafs but the function names are totally unreadable. I guess you could name them sgx_encl_dbg_read sgx_encl_dbg_write and leave the lowlevel helpers like the insn names. > +static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr, > + void *buf, int len, int write) > +{ > + struct sgx_encl *encl = vma->vm_private_data; > + struct sgx_encl_page *entry = NULL; > + char data[sizeof(unsigned long)]; > + unsigned long align; > + unsigned int flags; > + int offset; > + int cnt; > + int ret = 0; > + int i; > + > + /* If process was forked, VMA is still there but vm_private_data is set > + * to NULL. > + */ Kernel comments style is: /* * A sentence ending with a full-stop. * Another sentence. ... * More sentences. ... */ > + if (!encl) > + return -EFAULT; > + > + flags = atomic_read(&encl->flags); > + > + if (!(flags & SGX_ENCL_DEBUG) || !(flags & SGX_ENCL_INITIALIZED) || > + (flags & SGX_ENCL_DEAD)) > + return -EFAULT; > + > + for (i = 0; i < len; i += cnt) { > + entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK); > + if (IS_ERR(entry)) { > + ret = PTR_ERR(entry); > + break; > + } > + > + align = ALIGN_DOWN(addr + i, sizeof(unsigned long)); > + offset = (addr + i) & (sizeof(unsigned long) - 1); > + cnt = sizeof(unsigned long) - offset; > + cnt = min(cnt, len - i); > + > + ret = sgx_edbgrd(encl, entry, align, data); > + if (ret) > + goto out; > + > + if (write) { > + memcpy(data + offset, buf + i, cnt); > + ret = sgx_edbgwr(encl, entry, align, data); > + if (ret) > + goto out; > + } else } else { > + memcpy(buf + i, data + offset, cnt); } Put the else branch in {} too.
On Tue, Sep 22, 2020 at 05:44:24PM +0200, Borislav Petkov wrote: > > > Subject: Re: [PATCH v38 17/24] x86/sgx: ptrace() support for the SGX driver > ... x86/sgx: Add ptrace() support... > > subject needs a verb. > > On Tue, Sep 15, 2020 at 02:28:35PM +0300, Jarkko Sakkinen wrote: > > Add VMA callbacks for ptrace() that can be used with debug enclaves. > > With debug enclaves data can be read and write the memory word at a time > > I think you wanna say here > > "... data can be read and/or written a memory word at a time by using..." I also fixed the other issues but I'll paste here the rewrite that I did for the commit message: " x86/sgx: Add ptrace() support for the SGX driver Intel Sofware Guard eXtensions (SGX) allows creation of executable blobs called enclaves, which cannot be accessed by default when not executing inside the enclave. Enclaves can be entered by only using predefined memory addresses, which are defined the enclave is loaded. However, enclaves can defined as debug enclaves during the load time. In debug enclaves data can be read and/or written a memory word at a time by using by using ENCLS[EDBGRD] and ENCLS[EDBGWR] leaf instructions. Add 'access' implementation to vm_ops with the help of these functions. This allows to use ptrace() with debug enclaves. " I also think that mm CC would make sense also for this patch. /Jarkko
On Wed, Sep 23, 2020 at 04:20:49PM +0300, Jarkko Sakkinen wrote: > Intel Sofware Guard eXtensions (SGX) allows creation of executable blobs > called enclaves, which cannot be accessed by default when not executing > inside the enclave. Enclaves can be entered by only using predefined memory > addresses, which are defined the enclave is loaded. ^ "when" or "before". I think it is before. > However, enclaves can defined as debug enclaves during the load time. In "However, enclaves can be defined as debug enclaves at load time." > debug enclaves data can be read and/or written a memory word at a time by > using by using ENCLS[EDBGRD] and ENCLS[EDBGWR] leaf instructions. only one "by using" is enough. > Add 'access' implementation to vm_ops with the help of these functions. "Add an ->access virtual MM function for accessing the enclave's memory... "
On Wed, Sep 23, 2020 at 06:17:33PM +0200, Borislav Petkov wrote: > > Add 'access' implementation to vm_ops with the help of these functions. > > "Add an ->access virtual MM function for accessing the enclave's memory... " Thank you. I wrote the last paragraph like this: "Add an '->access' virtual function for accessing the enclave's memory to vm_ops by using these functions. This allows to use ptrace() with debug enclaves." > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
On Thu, Sep 24, 2020 at 02:51:28PM +0300, Jarkko Sakkinen wrote: > On Wed, Sep 23, 2020 at 06:17:33PM +0200, Borislav Petkov wrote: > > > Add 'access' implementation to vm_ops with the help of these functions. > > > > "Add an ->access virtual MM function for accessing the enclave's memory... " > > Thank you. I wrote the last paragraph like this: > > "Add an '->access' virtual function for accessing the enclave's memory > to vm_ops by using these functions. This allows to use ptrace() with "to vm_ops" must come after "function". But lemme ask what is "vm_ops"?
On Thu, Sep 24, 2020 at 05:57:51PM +0200, Borislav Petkov wrote: > On Thu, Sep 24, 2020 at 02:51:28PM +0300, Jarkko Sakkinen wrote: > > On Wed, Sep 23, 2020 at 06:17:33PM +0200, Borislav Petkov wrote: > > > > Add 'access' implementation to vm_ops with the help of these functions. > > > > > > "Add an ->access virtual MM function for accessing the enclave's memory... " > > > > Thank you. I wrote the last paragraph like this: > > > > "Add an '->access' virtual function for accessing the enclave's memory > > to vm_ops by using these functions. This allows to use ptrace() with > > "to vm_ops" must come after "function". > > But lemme ask what is "vm_ops"? I assume this is a rethorical question and I notice what I suggested looks as bad as my earlier commit message :-) So, I gave it some thought that and decided to "open code" the paragraph as "Add sgx_vma_access() function that implements 'access' virtual function of struct vm_operations_struct. Use formentioned leaf instructions to achieve read and write primitives for the enclave memory." I think this starts to have the right balance and is understandable. Still open for futher suggestion of course. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
On Thu, Sep 24, 2020 at 11:39:07PM +0300, Jarkko Sakkinen wrote: > On Thu, Sep 24, 2020 at 05:57:51PM +0200, Borislav Petkov wrote: > > On Thu, Sep 24, 2020 at 02:51:28PM +0300, Jarkko Sakkinen wrote: > > > On Wed, Sep 23, 2020 at 06:17:33PM +0200, Borislav Petkov wrote: > > > > > Add 'access' implementation to vm_ops with the help of these functions. > > > > > > > > "Add an ->access virtual MM function for accessing the enclave's memory... " > > > > > > Thank you. I wrote the last paragraph like this: > > > > > > "Add an '->access' virtual function for accessing the enclave's memory > > > to vm_ops by using these functions. This allows to use ptrace() with > > > > "to vm_ops" must come after "function". > > > > But lemme ask what is "vm_ops"? > > I assume this is a rethorical question and I notice what I suggested > looks as bad as my earlier commit message :-) > > So, I gave it some thought that and decided to "open code" the paragraph > as > > "Add sgx_vma_access() function that implements 'access' virtual function > of struct vm_operations_struct. Use formentioned leaf instructions to > achieve read and write primitives for the enclave memory." > > I think this starts to have the right balance and is understandable. > > Still open for futher suggestion of course. I'm not sure if I said it already but I also added cc to linux-mm (same CC's in the patch as with mprotect callback commit). This should also have mm ack I think. /Jarkko
On Thu, Sep 24, 2020 at 11:38:59PM +0300, Jarkko Sakkinen wrote: > I assume this is a rethorical question Of course - our text should not be write-only. > and I notice what I suggested > looks as bad as my earlier commit message :-) > > So, I gave it some thought that and decided to "open code" the paragraph > as > > "Add sgx_vma_access() function that implements 'access' virtual function > of struct vm_operations_struct. Use formentioned leaf instructions to "aforementioned" > achieve read and write primitives for the enclave memory." I'd say "to provide read and write access to enclave memory."
On Thu, Sep 24, 2020 at 11:40:22PM +0300, Jarkko Sakkinen wrote: > I'm not sure if I said it already but I also added cc to linux-mm (same > CC's in the patch as with mprotect callback commit). This should also > have mm ack I think. Why? This is adding ptrace functionality to enclaves which is purely arch/x86/.
On Fri, Sep 25, 2020 at 09:53:01AM +0200, Borislav Petkov wrote: > On Thu, Sep 24, 2020 at 11:40:22PM +0300, Jarkko Sakkinen wrote: > > I'm not sure if I said it already but I also added cc to linux-mm (same > > CC's in the patch as with mprotect callback commit). This should also > > have mm ack I think. > > Why? This is adding ptrace functionality to enclaves which is purely > arch/x86/. You are right. No changes to vm_operations_struct itself. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
On Fri, Sep 25, 2020 at 09:51:04AM +0200, Borislav Petkov wrote: > On Thu, Sep 24, 2020 at 11:38:59PM +0300, Jarkko Sakkinen wrote: > > I assume this is a rethorical question > > Of course - our text should not be write-only. > > > and I notice what I suggested > > looks as bad as my earlier commit message :-) > > > > So, I gave it some thought that and decided to "open code" the paragraph > > as > > > > "Add sgx_vma_access() function that implements 'access' virtual function > > of struct vm_operations_struct. Use formentioned leaf instructions to > > "aforementioned" Ugh, that the was worst spelling I've done for a while. > > achieve read and write primitives for the enclave memory." > > I'd say "to provide read and write access to enclave memory." Sound better yes, thanks. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette Updated the commit message accordingly. I also rebased now to x86 tip (and yes, merge conflicts were trivial to sort out). /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 11ec2df59b54..7f8df2c8ef35 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -333,10 +333,97 @@ static int sgx_vma_mprotect(struct vm_area_struct *vma, return mprotect_fixup(vma, pprev, start, end, newflags); } +static int sgx_edbgrd(struct sgx_encl *encl, struct sgx_encl_page *page, + unsigned long addr, void *data) +{ + unsigned long offset = addr & ~PAGE_MASK; + int ret; + + + ret = __edbgrd(sgx_get_epc_addr(page->epc_page) + offset, data); + if (ret) + return -EIO; + + return 0; +} + +static int sgx_edbgwr(struct sgx_encl *encl, struct sgx_encl_page *page, + unsigned long addr, void *data) +{ + unsigned long offset = addr & ~PAGE_MASK; + int ret; + + ret = __edbgwr(sgx_get_epc_addr(page->epc_page) + offset, data); + if (ret) + return -EIO; + + return 0; +} + +static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr, + void *buf, int len, int write) +{ + struct sgx_encl *encl = vma->vm_private_data; + struct sgx_encl_page *entry = NULL; + char data[sizeof(unsigned long)]; + unsigned long align; + unsigned int flags; + int offset; + int cnt; + int ret = 0; + int i; + + /* If process was forked, VMA is still there but vm_private_data is set + * to NULL. + */ + if (!encl) + return -EFAULT; + + flags = atomic_read(&encl->flags); + + if (!(flags & SGX_ENCL_DEBUG) || !(flags & SGX_ENCL_INITIALIZED) || + (flags & SGX_ENCL_DEAD)) + return -EFAULT; + + for (i = 0; i < len; i += cnt) { + entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK); + if (IS_ERR(entry)) { + ret = PTR_ERR(entry); + break; + } + + align = ALIGN_DOWN(addr + i, sizeof(unsigned long)); + offset = (addr + i) & (sizeof(unsigned long) - 1); + cnt = sizeof(unsigned long) - offset; + cnt = min(cnt, len - i); + + ret = sgx_edbgrd(encl, entry, align, data); + if (ret) + goto out; + + if (write) { + memcpy(data + offset, buf + i, cnt); + ret = sgx_edbgwr(encl, entry, align, data); + if (ret) + goto out; + } else + memcpy(buf + i, data + offset, cnt); + +out: + mutex_unlock(&encl->lock); + + if (ret) + break; + } + + return ret < 0 ? ret : i; +} + const struct vm_operations_struct sgx_vm_ops = { .open = sgx_vma_open, .fault = sgx_vma_fault, .mprotect = sgx_vma_mprotect, + .access = sgx_vma_access, }; /**