implement COW on ghost memory

[XiaowanDong]

1. ghostmemCOW(): Invoked at fork. It copies the page table entries of ghost memory from the parent to the child. Also, it write-protects these page table entries.
2. Modify sva_ghost_fault() to check whether the page fault is due to a write access to a read-only ghost memory page. If yes, COW that ghost memory page.
3. Create a new field called flags in SVAThread to save the decision regarding whether to do COW on ghost memory or not at fork. The decision is made based on the arguments of the fork family syscalls, which are extracted in SVASyscall handler.
4. Pull out previous inline functions such as get_pml4eVaddr() from SVA/lib/mmu.c to SVA/include/sva/mmu.h so that they could be called in other files such as SVA/lib/secmem.c

[jtcriswell]

Xiaowan, can you remind me why you added the cr3 argument to sva_init_stack()? I don't recall why you needed it. I'd like to avoid adding it if possible because it requires us to update the FreeBSD kernel patch (which I'm currently trying to update with other changes you've made).

[jtcriswell]

Also, for future refactoring, it's best to make incremental commits and pull requests. For example, if you need to refactor some of the code, first do that, commit it, and make a pull request. After that, you can make more commits that add the feature, commit, and create a pull request. That allows me to review and merge your changes in smaller pieces and allows you to get more of your code merged in if some of it needs some additional discussion or modification.

[XiaowanDong]

John, could you please offer some advice regarding how we could get the cr3 of the new thread in sva_init_stack() without passing it as an argument?

…

On Fri, Sep 29, 2017 at 4:14 PM, John Criswell ***@***.***> wrote: Xiaowan, can you remind me why you added the cr3 argument to sva_init_stack()? I don't recall why you needed it. I'd like to avoid adding it if possible because it requires us to update the FreeBSD kernel patch (which I'm currently trying to update with other changes you've made). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23issuecomment-2D333240742&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=1l1OHw99mFCITJddGP3rRhkIReSdY6IefDGSqlxnwtY&s=_bIWowzqLXtHWKXvR-fPURAjLKDi42dhrr6iF1Jm2Gg&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AGPCudXJBkSdF0uME5k2U0VjsGjXLw7Oks5snV2dgaJpZM4Pn94p&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=1l1OHw99mFCITJddGP3rRhkIReSdY6IefDGSqlxnwtY&s=lsG3tQ47MYs9r6QmgrI4ElTfxtoaA5qMp8DKfKBYrU0&e=> .

[XiaowanDong]

Sure. I would keep it in mind. Sorry about the inconvenience.

…

On Fri, Sep 29, 2017 at 4:16 PM, John Criswell ***@***.***> wrote: Also, for future refactoring, it's best to make incremental commits and pull requests. For example, if you need to refactor some of the code, first do that, commit it, and make a pull request. After that, you can make more commits that add the feature, commit, and create a pull request. That allows me to review and merge your changes in smaller pieces and allows you to get more of your code merged in if some of it needs some additional discussion or modification. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23issuecomment-2D333241123&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=k5uBdvgN5C5LGbCr0IHzJLXSDzC9PboNlEqeN_Fb78k&s=Ef5Ir0b5d0haiW_bJKO2Xo52XbiZ_cAGof2rdL0g5TU&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AGPCudveifULojLRbN7Thi7y-2DQeRuKMPks5snV4YgaJpZM4Pn94p&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=k5uBdvgN5C5LGbCr0IHzJLXSDzC9PboNlEqeN_Fb78k&s=eNXSseaSkziuzKxWZCx5FxqN0sOGO4BWWGcWD1fYqQU&e=> .

[jtcriswell]

I think the question is whether you need the CR3 value. The SVAThread already has the PML4E that maps ghost memory into the process's virtual address space. Can that be used to write-protect the memory instead of using CR3?

[jtcriswell]

I've added some comments below. In addition to these changes, the kernel patch must be updated to reflect the new APIs. I'm working on getting some of your fixes from the FreeBSD90 repository into the patch; I think I can have that finished by end of day Monday. After that, I can merge in whatever changes you need to have to make this patch work. If you can modify the master branch in FreeBSD90 with the changes that go along with this patch, that will make it much easier for me to merge the code in.

[jtcriswell]

Nitpick: you misspelled "field" as "filed." FWIW, I make this mistake all the time (in fact, I made it and had to correct it while writing this comment. :)

[jtcriswell]

You should create a macro for the 0x7d80 constant. I assume it's some offset into a C structure.

[jtcriswell]

It looks like you're testing the flags argument to see if the ghost memory should be copy-on-write. Is that correct? If so, we're adding overhead to the critical path (system call dispatch). The flags argument of the system call is already saved in the Interrupt Context; we could have sva_init_stack() just check the flags there to determine if the ghost memory should be marked copy-on-write. Adding overhead to sva_init_stack would be better than adding overhead to system call dispatch.

[jtcriswell]

Is this where you're using the CR3 field of the Integer State? If so, why not use the secmemPML4e in the SVAThread to find the page table entries that map ghost memory?

[XiaowanDong]

ghostmemCOW() is called by sva_init_stack() at fork, where the new thread (child)'s SVAThread->secmemPML4e is set to be equal to the old thread's. The new thread's secmemPML4e is not set until in sva_ghost_fault() or ghostMalloc() when the ghost memory is allocated at the first time. Therefore I am not able to know the new child's pml4 at fork without passing it from the OS.

[jtcriswell]

Please be sure to indent code within blocks. It looks like the code within the "if" clause isn't indented as it should be.

Please also be sure to indent using two spaces instead of using tabs and to wrap lines at 80 columns. We are (kind of) following the LLVM Coding Standards in SVA.

[jtcriswell]

Could you add a function-level comment for sva_ghost_fault()? I neglected to write one when I added sva_ghost_fault(). :(

[jtcriswell]

I think this code should use the new frame cache that Zhuojia added instead of calling the OS callback function directly. That will enable us to use cached frames; it will also make it easier to mitigate side channels made possible via the copy-on-write mechanism.

[jtcriswell]

This code should have a comment explaining what it is doing. Also, the constant should be replaced with a const global variable that makes it clear what the constant represents.

[XiaowanDong]

On Sat, Sep 30, 2017 at 11:24 AM, John Criswell ***@***.***> wrote: ***@***.**** requested changes on this pull request. I've added some comments below. In addition to these changes, the kernel patch must be updated to reflect the new APIs. I'm working on getting some of your fixes from the FreeBSD90 repository into the patch; I think I can have that finished by end of day Monday. After that, I can merge in whatever changes you need to have to make this patch work. If you can modify the master branch in FreeBSD90 with the changes that go along with this patch, that will make it much easier for me to merge the code in.

I have pushed my latest changes to xiaowan-src branch of FreeBSD90. Please see the inline comment below regarding the change to sva_init_stack() which could affects the kernel patch.

------------------------------

In SVA/lib/handlers.S <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009072&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=AtMQUNoukPJLoWtI1AOtOWHs_nPrynlrqfBOTGy5Ieg&e=> : > @@ -507,24 +507,58 @@ SVAsyscall: * if this is the fork(), vfork(), rfork(), or pdfork() system calls. The * system call number will be in %rax and can have the following values * (you can find these in syscalls.master) within the FreeBSD kernel source - * code): + * code). + * According to the type of fork and the flags, we make a decision about + * whether to COW on ghost memory or not, and save the decision in the flags + * filed of the current SVAThread. (1 is COW, and 0 is not COW.) Nitpick: you misspelled "field" as "filed." FWIW, I make this mistake all the time (in fact, I made it and had to correct it while writing this comment. :) ------------------------------ In SVA/lib/handlers.S <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009097&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=iIurBZOd5NTKKwjzt0EwODMUofgCl5ll1usVXSeiuNs&e=> : > * * 2 - fork() * 66 - vfork() * 251 - rfork() * 518 - pdfork() */ + + /* Find the flags field of the current SVAThread */ + movq %gs:0x260, %r14 + movq CPU_THREAD(%r14), %r14 + movq $0, 0x7d80(%r14) You should create a macro for the 0x7d80 constant. I assume it's some offset into a C structure. ------------------------------ In SVA/lib/handlers.S <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009145&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=_XBEQc7zJjATvOBA_TZo44Bi42IgmE_1QTGIqEEp-To&e=> : > cmpq $251, IC_RAX(%rsp) - cmoveq %r13, %r12 + jne NEXT_TEST_FORK + movq %r13, %r12 + /* test the syscall argument flags, whether It looks like you're testing the flags argument to see if the ghost memory should be copy-on-write. Is that correct? If so, we're adding overhead to the critical path (system call dispatch). The flags argument of the system call is already saved in the Interrupt Context; we could have sva_init_stack() just check the flags there to determine if the ghost memory should be marked copy-on-write. Adding overhead to sva_init_stack would be better than adding overhead to system call dispatch. ------------------------------ In SVA/lib/mmu.c <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009196&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=zb6O0NRG2vgcRe3pp5oB2js-InMJWhb-Kn74OevK_UI&e=> : > +void +ghostmemCOW(struct SVAThread* oldThread, struct SVAThread* newThread) +{ + + uintptr_t vaddr_start, vaddr_end, size; + + vaddr_start = (uintptr_t) SECMEMSTART; + size = oldThread->secmemSize; + vaddr_end = vaddr_start + size; + + /* + * Get the PML4E of the new process's page table. If there isn't one in the + * table, add one. + */ + sva_integer_state_t integerState = newThread->integerState; + pml4e_t * pml4e = (pml4e_t *) getVirtual(integerState.cr3 + secmemOffset); Is this where you're using the CR3 field of the Integer State? If so, why not use the secmemPML4e in the SVAThread to find the page table entries that map ghost memory?

Currently, the secmemPML4e of the new process (the child) is set to be the same as the old process (the parent) at fork time, therefore we are not be able to infer the pml4e of the new process mapping ghost memory based on the secmemPML4e field.

…

------------------------------ In SVA/lib/mmu.c <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009243&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=4gsiyy3Fa5V7q7TPXEQmQNwjLrY6jJh0nSCe2YcPK1o&e=> : > + uintptr_t vaddr_start, vaddr_end, size; + + vaddr_start = (uintptr_t) SECMEMSTART; + size = oldThread->secmemSize; + vaddr_end = vaddr_start + size; + + /* + * Get the PML4E of the new process's page table. If there isn't one in the + * table, add one. + */ + sva_integer_state_t integerState = newThread->integerState; + pml4e_t * pml4e = (pml4e_t *) getVirtual(integerState.cr3 + secmemOffset); + + unprotect_paging(); + if (!isPresent (pml4e)) { + /* Page table page index */ Please be sure to indent code within blocks. It looks like the code within the "if" clause isn't indented as it should be. Please also be sure to indent using two spaces instead of using tabs and to wrap lines at 80 columns. We are (kind of) following the LLVM Coding Standards in SVA. ------------------------------ In SVA/lib/secmem.c <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009288&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=oha0Es1p5yN4OLNwOw_6rJ8CcbKzP1KvG0EhuvfSwoU&e=> : > @@ -502,7 +503,7 @@ freeSecureMemory (void) { } void -sva_ghost_fault (uintptr_t vaddr) { +sva_ghost_fault (uintptr_t vaddr, unsigned long code) { Could you add a function-level comment for sva_ghost_fault()? I neglected to write one when I added sva_ghost_fault(). :( ------------------------------ In SVA/lib/secmem.c <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009429&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=3bhS4DQrAiUiqI4ymYpdlvmq-zTx2rRMeALmR8ewmeE&e=> : > + + if(pgDesc->type != PG_GHOST) + panic("SVA: sva_ghost_fault: vaddr = 0x%lx paddr = 0x%lx is not a ghost memory page!\n", vaddr, paddr); + /* If only one process maps this page, directly grant this process write permission */ + + + unprotect_paging(); + if(pgDesc->count == 1) + { + * pte = (* pte) | PTE_CANWRITE; + } + /* Otherwise copy-on-write */ + else + { + uintptr_t vaddr_old = (uintptr_t) getVirtual(paddr); + uintptr_t paddr_new = provideSVAMemory (X86_PAGE_SIZE); I think this code should use the new frame cache that Zhuojia added instead of calling the OS callback function directly. That will enable us to use cached frames; it will also make it easier to mitigate side channels made possible via the copy-on-write mechanism. ------------------------------ In SVA/lib/state.c <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009498&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=EznP_nIKqpaTfLFwWwcabHKzgY1I9XJKTFa-Rd8FRdg&e=> : > @@ -1284,6 +1288,11 @@ sva_init_stack (unsigned char * start_stackp, icontextp->valid |= IC_is_valid; } + if(vg && (oldThread->secmemSize) && (oldThread->flags & 1)) This code should have a comment explaining what it is doing. Also, the constant should be replaced with a const global variable that makes it clear what the constant represents. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23pullrequestreview-2D66316288&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=bHsqiZRcKNKmF7oKldX4MrN3D5ElcTxdC1EKGQ5tC3E&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AGPCua9lCgfIWSEfGOwEOfeuz12AHt8Bks5snms5gaJpZM4Pn94p&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=68uNSqL_gYy1x8Ng-BD65Qv_DJyDh0oOMj4ZwRpZnP0&e=> .

[jtcriswell]

On 10/2/17 12:47 AM, XiaowanDong wrote: [snip] > > +void > +ghostmemCOW(struct SVAThread* oldThread, struct SVAThread* newThread) > +{ > + > + uintptr_t vaddr_start, vaddr_end, size; > + > + vaddr_start = (uintptr_t) SECMEMSTART; > + size = oldThread->secmemSize; > + vaddr_end = vaddr_start + size; > + > + /* > + * Get the PML4E of the new process's page table. If there isn't one in the > + * table, add one. > + */ > + sva_integer_state_t integerState = newThread->integerState; > + pml4e_t * pml4e = (pml4e_t *) getVirtual(integerState.cr3 + secmemOffset); > > Is this where you're using the CR3 field of the Integer State? If so, why > not use the secmemPML4e in the SVAThread to find the page table entries > that map ghost memory? > Currently, the secmemPML4e of the new process (the child) is set to be the same as the old process (the parent) at fork time, therefore we are not be able to infer the pml4e of the new process mapping ghost memory based on the secmemPML4e field.

There's two issues here. First, allowing the OS to specify the top-level page table page for the child thread opens up the possibility for malicious behavior; the OS could specify an existing page table entry or some random address that it wants SVA to corrupt. The new_cr3 value would need to be validated as if it declared as a top-level page table page. Second, it's still not clear to me why you need the CR3 of the new process. What I think you want to do is to make a copy of the PDPTE that maps ghost memory for the child and mark both the parent's ghost memory PDPTE entries and the child's PDPTE entries read-only. It looks like you already have logic to do this in ghostmemCOW(). If you change the PML4E entry in the child's SVAThread to point to the new PDPTE for the child, sva_swap_integer will install it into the top-level page table page on the context switch, and sva_mm_load_pagetable() will preserve it when switching page tables. Regards, John Criswell

…

> ------------------------------ > > In SVA/lib/mmu.c > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009243&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=4gsiyy3Fa5V7q7TPXEQmQNwjLrY6jJh0nSCe2YcPK1o&e=> > : > > > + uintptr_t vaddr_start, vaddr_end, size; > + > + vaddr_start = (uintptr_t) SECMEMSTART; > + size = oldThread->secmemSize; > + vaddr_end = vaddr_start + size; > + > + /* > + * Get the PML4E of the new process's page table. If there isn't one in the > + * table, add one. > + */ > + sva_integer_state_t integerState = newThread->integerState; > + pml4e_t * pml4e = (pml4e_t *) getVirtual(integerState.cr3 + secmemOffset); > + > + unprotect_paging(); > + if (!isPresent (pml4e)) { > + /* Page table page index */ > > Please be sure to indent code within blocks. It looks like the code within > the "if" clause isn't indented as it should be. > > Please also be sure to indent using two spaces instead of using tabs and > to wrap lines at 80 columns. We are (kind of) following the LLVM Coding > Standards in SVA. > ------------------------------ > > In SVA/lib/secmem.c > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009288&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=oha0Es1p5yN4OLNwOw_6rJ8CcbKzP1KvG0EhuvfSwoU&e=> > : > > > @@ -502,7 +503,7 @@ freeSecureMemory (void) { > } > > void > -sva_ghost_fault (uintptr_t vaddr) { > +sva_ghost_fault (uintptr_t vaddr, unsigned long code) { > > Could you add a function-level comment for sva_ghost_fault()? I neglected > to write one when I added sva_ghost_fault(). :( > ------------------------------ > > In SVA/lib/secmem.c > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009429&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=3bhS4DQrAiUiqI4ymYpdlvmq-zTx2rRMeALmR8ewmeE&e=> > : > > > + > + if(pgDesc->type != PG_GHOST) > + panic("SVA: sva_ghost_fault: vaddr = 0x%lx paddr = 0x%lx is not a ghost memory page!\n", vaddr, paddr); > + /* If only one process maps this page, directly grant this process write permission */ > + > + > + unprotect_paging(); > + if(pgDesc->count == 1) > + { > + * pte = (* pte) | PTE_CANWRITE; > + } > + /* Otherwise copy-on-write */ > + else > + { > + uintptr_t vaddr_old = (uintptr_t) getVirtual(paddr); > + uintptr_t paddr_new = provideSVAMemory (X86_PAGE_SIZE); > > I think this code should use the new frame cache that Zhuojia added > instead of calling the OS callback function directly. That will enable us > to use cached frames; it will also make it easier to mitigate side channels > made possible via the copy-on-write mechanism. > ------------------------------ > > In SVA/lib/state.c > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23discussion-5Fr142009498&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=EznP_nIKqpaTfLFwWwcabHKzgY1I9XJKTFa-Rd8FRdg&e=> > : > > > @@ -1284,6 +1288,11 @@ sva_init_stack (unsigned char * start_stackp, > icontextp->valid |= IC_is_valid; > } > > + if(vg && (oldThread->secmemSize) && (oldThread->flags & 1)) > > This code should have a comment explaining what it is doing. Also, the > constant should be replaced with a const global variable that makes it > clear what the constant represents. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jtcriswell_SVA_pull_24-23pullrequestreview-2D66316288&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=bHsqiZRcKNKmF7oKldX4MrN3D5ElcTxdC1EKGQ5tC3E&e=>, > or mute the thread > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AGPCua9lCgfIWSEfGOwEOfeuz12AHt8Bks5snms5gaJpZM4Pn94p&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=9lVsUv0m4rDQCgSupIkpgbizkVWsSr2-oDgbaUgY_C0&m=xZn5lUBnHmeSLCcip05k6lCFadK6g7E2R1yDtT8laXk&s=68uNSqL_gYy1x8Ng-BD65Qv_DJyDh0oOMj4ZwRpZnP0&e=> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEaN27WzCcRZwYREMtzejA2TQPmY_Gpyks5soGrXgaJpZM4Pn94p>.

-- John Criswell Assistant Professor Department of Computer Science, University of Rochester http://www.cs.rochester.edu/u/criswell

[XiaowanDong]

I have modified the code according to John's comments above. This pull request is ready to be reviewed again.

[XiaowanDong]

The changes have been made as requested. It is now ready to be reviewed again.

[XiaowanDong]

This line would not work since provideSVAMemory() has already removed the kernel's direct mapping for this page when it becomes a ghost page. The only way to deal with this problem is to port the SVA DMAP from SVAMPX/mmu-devel branch to the main branch. Please let me know what you think @jtcriswell .