[RFC] core: make pager aliased paged not always writable #1551
Conversation
|
Won't this have a big performance impact? (due to the TLB flush) |
core/arch/arm/mm/tee_pager.c
Outdated
|
|
||
| DMSG("0x%" PRIxPA, pa); | ||
|
|
||
| if (!pager_alias_next_free || !ti->num_entries) | ||
| panic("invalid alias entry"); | ||
|
|
||
| #ifndef CFG_CORE_RWDATA_NOEXEC |
There was a problem hiding this comment.
I'd rather try to protect as much as possible in the pager. So if no one objects please remove all these ifdefs in this patch.
There was a problem hiding this comment.
I do not object. But your suggestion tends me to believe even CFG_CORE_RWDATA_NOEXEC configuration switch should be discarded and related code always embedded.
There was a problem hiding this comment.
Fine with me to remove CFG_CORE_RWDATA_NOEXEC.
There was a problem hiding this comment.
Ok. I propose that (enforce seperation of rx and rw memory) in a specific commit, hence a specifc PR. Meanwhile, i'll update this PR to remove this condition in the pager only.
|
I don't think you'll notice it. But I think we should to something about all those TODOs, that could actually improve the performance a bit. |
I have tested on my HW and impact was quite slow, but I did not test with a stressed setup on a real usecase... That said, pager already flushes TLB when updating the mmu. Maybe this would deserve some benchmark tests :) |
|
@jenswi-linaro @etienne-lms OK, I suppose we can deal with that in a separate patch. |
| attr_alias |= TEE_MATTR_PW; | ||
| core_mmu_set_entry(ti, idx_alias, pa_alias, attr_alias); | ||
| /* TODO: flush TLB for target page only */ | ||
| core_tlb_maintenance(TLBINV_UNIFIEDTLB, 0); |
There was a problem hiding this comment.
If we always made sure that aliased pages aren't accessible unless in this function there wouldn't be any need for this flush. But that's something for another PR.
There was a problem hiding this comment.
made sure that aliased pages aren't accessible unless in this function
Any idea ? Seems hard to implement. Through kasan?
core/arch/arm/mm/tee_pager.c
Outdated
| @@ -484,6 +485,21 @@ static void tee_pager_load_page(struct tee_pager_area *area, vaddr_t page_va, | |||
| { | |||
| size_t idx = (page_va - area->base) >> SMALL_PAGE_SHIFT; | |||
| const void *stored_page = area->store + idx * SMALL_PAGE_SIZE; | |||
| struct core_mmu_table_info *ti __maybe_unused; | |||
There was a problem hiding this comment.
The __maybe_unused attributes can be removed now.
|
Comments addressed. This change protects alias pages related the read-only (potentially read-exec) memory. Should it be extended to read/write memory ? Such should also not be accessed from aliased vmem. Travis failure due to issue in optee_client, fixed through OP-TEE/optee_client#91. |
|
|
|
Tags applied. |
|
Travis complains on trace log size. Has something changed about it ? Or is my PR buggy in some way ? |
No, it's just that
I don't know, in the raw log we can see that the failing test case is: |
|
ok, something about the last changes merged in optee_client. |
|
@etienne-lms yeah, the solution is probably to remove the tests that are trying to allocate TEEC_CONFIG_SHAREDMEM_MAX_SIZE more or less. |
|
rebased. travis at work. |
This change lower the attack surface of executable memory in the pager by allowing write access to aliased virtual pages related to read-only content (including executable content) only when pager needs to update page content. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Tested-by: Etienne Carriere <etienne.carriere@linaro.org> (qemu_virt) Tested-by: Etienne Carriere <etienne.carriere@st.com> (b2260)
|
rebased. |
This change lowers the attack surface of executable memory in the pager by allowing write access to aliased virtual pages related to read-only content (including executable content) only when pager needs to update page content.