From 0c67d0838a040836b7671625fbadd0595a8aafe6 Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Thu, 17 Feb 2022 13:50:05 +0100 Subject: [PATCH] hw: restrict page-table lookup to rw-pages The `lookup_translation` function got introduced and is used only in the context to proof whether a cache maintainance function can be executed safely by the kernel. Unfortunately, it did not checked write permissions, which can lead to permission faults. This commit restricts the lookup function to only succeed when the target page is writeable. Consequently, the lookup function gets renamed to `lookup_rw_translation`. Fix genodelabs/genode#4348 --- repos/base-hw/src/core/platform_pd.cc | 4 ++-- repos/base-hw/src/core/platform_pd.h | 4 ++-- .../src/core/spec/arm/kernel/thread_caches.cc | 10 +++++----- repos/base-hw/src/include/hw/spec/arm/lpae.h | 20 ++++++++++++------- .../src/include/hw/spec/arm/page_table.h | 19 +++++++++++------- .../src/include/hw/spec/riscv/page_table.h | 2 +- .../src/include/hw/spec/x86_64/page_table.h | 2 +- 7 files changed, 36 insertions(+), 25 deletions(-) diff --git a/repos/base-hw/src/core/platform_pd.cc b/repos/base-hw/src/core/platform_pd.cc index 1913837baa..3d258c1d3d 100644 --- a/repos/base-hw/src/core/platform_pd.cc +++ b/repos/base-hw/src/core/platform_pd.cc @@ -70,14 +70,14 @@ bool Hw::Address_space::insert_translation(addr_t virt, addr_t phys, } -bool Hw::Address_space::lookup_translation(addr_t const virt, addr_t & phys) +bool Hw::Address_space::lookup_rw_translation(addr_t const virt, addr_t & phys) { /** FIXME: for the time-being we use it without lock, * because it is used directly by the kernel when cache_coherent_region * gets called. In future it would be better that core provides an API * for it, and does the lookup with the hold lock */ - return _tt.lookup_translation(virt, phys, _tt_alloc); + return _tt.lookup_rw_translation(virt, phys, _tt_alloc); } diff --git a/repos/base-hw/src/core/platform_pd.h b/repos/base-hw/src/core/platform_pd.h index 979efb857d..db3383713e 100644 --- a/repos/base-hw/src/core/platform_pd.h +++ b/repos/base-hw/src/core/platform_pd.h @@ -122,8 +122,8 @@ class Hw::Address_space : public Genode::Address_space bool insert_translation(Genode::addr_t virt, Genode::addr_t phys, Genode::size_t size, Genode::Page_flags flags); - bool lookup_translation(Genode::addr_t const virt, - Genode::addr_t & phys); + bool lookup_rw_translation(Genode::addr_t const virt, + Genode::addr_t & phys); /***************************** ** Address-space interface ** diff --git a/repos/base-hw/src/core/spec/arm/kernel/thread_caches.cc b/repos/base-hw/src/core/spec/arm/kernel/thread_caches.cc index 3387f6620b..2eee9b912b 100644 --- a/repos/base-hw/src/core/spec/arm/kernel/thread_caches.cc +++ b/repos/base-hw/src/core/spec/arm/kernel/thread_caches.cc @@ -37,15 +37,15 @@ static void for_cachelines(addr_t base, } /** - * Lookup whether the page is backed, and if so make the memory coherent - * in between I-, and D-cache + * Lookup whether the page is backed and writeable, + * and if so make the memory coherent in between I-, and D-cache */ addr_t phys = 0; - if (thread.pd().platform_pd().lookup_translation(base, phys)) { + if (thread.pd().platform_pd().lookup_rw_translation(base, phys)) { fn(base, size); } else { - Genode::raw(thread, " tried to make invalid address ", - (void*)base, " cache coherent"); + Genode::raw(thread, " tried to do cache maintainance at ", + "unallowed address ", (void*)base); } } diff --git a/repos/base-hw/src/include/hw/spec/arm/lpae.h b/repos/base-hw/src/include/hw/spec/arm/lpae.h index a5d5ed90bb..d898e37404 100644 --- a/repos/base-hw/src/include/hw/spec/arm/lpae.h +++ b/repos/base-hw/src/include/hw/spec/arm/lpae.h @@ -388,7 +388,10 @@ class Hw::Level_3_translation_table : if (!Descriptor::valid(desc)) return; phys = Block_descriptor::Output_address::masked(desc); - found = true; + typename Block_descriptor::access_t ap = + Block_descriptor::Access_permission::get(desc); + found = ap == Block_descriptor::Access_permission::PRIVILEGED_RW || + ap == Block_descriptor::Access_permission::USER_RW; } }; @@ -409,7 +412,7 @@ class Hw::Level_3_translation_table : _range_op(vo, pa, size, Remove_func()); } - bool lookup_translation(addr_t vo, addr_t & pa, Allocator&) + bool lookup_rw_translation(addr_t vo, addr_t & pa, Allocator&) { size_t page_size = 1 << SIZE_LOG2_4KB; Lookup_func functor {}; @@ -542,15 +545,18 @@ class Hw::Level_x_translation_table : case Descriptor::BLOCK: { phys = Block_descriptor::Output_address::masked(desc); - found = true; + typename Block_descriptor::access_t ap = + Block_descriptor::Access_permission::get(desc); + found = ap == Block_descriptor::Access_permission::PRIVILEGED_RW || + ap == Block_descriptor::Access_permission::USER_RW; return; }; case Descriptor::TABLE: { /* use allocator to retrieve virt address of table */ E & table = alloc.virt_addr(Nt::masked(desc)); - found = table.lookup_translation(vo - (vo & Base::BLOCK_MASK), - phys, alloc); + found = table.lookup_rw_translation(vo - (vo & Base::BLOCK_MASK), + phys, alloc); return; }; case Descriptor::INVALID: return; @@ -596,13 +602,13 @@ class Hw::Level_x_translation_table : } /** - * Lookup translation + * Lookup writeable translation * * \param virt region offset within the tables virtual region * \param phys region size * \param alloc second level translation table allocator */ - bool lookup_translation(addr_t const virt, addr_t & phys, + bool lookup_rw_translation(addr_t const virt, addr_t & phys, Allocator & alloc) { size_t page_size = 1 << SIZE_LOG2_4KB; diff --git a/repos/base-hw/src/include/hw/spec/arm/page_table.h b/repos/base-hw/src/include/hw/spec/arm/page_table.h index f3bfce9272..3d66e031a3 100644 --- a/repos/base-hw/src/include/hw/spec/arm/page_table.h +++ b/repos/base-hw/src/include/hw/spec/arm/page_table.h @@ -261,13 +261,13 @@ class Hw::Page_table } /** - * Lookup translation + * Lookup writeable translation * * \param virt virtual address offset to look for * \param phys physical address to return * \returns true if lookup was successful, otherwise false */ - bool lookup_translation(addr_t const virt, addr_t & phys) + bool lookup_rw_translation(addr_t const virt, addr_t & phys) { unsigned idx = 0; if (!_index_by_vo(idx, virt)) return false; @@ -275,8 +275,10 @@ class Hw::Page_table switch (Descriptor::type(_entries[idx])) { case Descriptor::SMALL_PAGE: { + Small_page::access_t ap = + Small_page::Ap::get(_entries[idx]); phys = Small_page::Pa::masked(_entries[idx]); - return true; + return ap == 1 || ap == 3; } default: return false; } @@ -607,14 +609,15 @@ class Hw::Page_table } /** - * Lookup translation + * Lookup writeable translation * * \param virt virtual address to look at * \param phys physical address to return * \param alloc second level translation table allocator * \returns true if a translation was found, otherwise false */ - bool lookup_translation(addr_t const virt, addr_t & phys, Allocator & alloc) + bool lookup_rw_translation(addr_t const virt, addr_t & phys, + Allocator & alloc) { unsigned idx = 0; if (!_index_by_vo(idx, virt)) return false; @@ -623,8 +626,10 @@ class Hw::Page_table case Descriptor::SECTION: { + Section::access_t ap = + Section::Ap::get(_entries[idx]); phys = Section::Pa::masked(_entries[idx]); - return true; + return ap == 1 || ap == 3; } case Descriptor::PAGE_TABLE: @@ -636,7 +641,7 @@ class Hw::Page_table alloc.virt_addr(Ptd::Pa::masked(_entries[idx])); addr_t const offset = virt - Section::Pa::masked(virt); - return pt.lookup_translation(offset, phys); + return pt.lookup_rw_translation(offset, phys); } default: return false; }; diff --git a/repos/base-hw/src/include/hw/spec/riscv/page_table.h b/repos/base-hw/src/include/hw/spec/riscv/page_table.h index 7848d18e68..d95504b5c7 100644 --- a/repos/base-hw/src/include/hw/spec/riscv/page_table.h +++ b/repos/base-hw/src/include/hw/spec/riscv/page_table.h @@ -351,7 +351,7 @@ class Sv39::Level_x_translation_table _range_op(vo, 0, size, Remove_func(alloc)); } - bool lookup_translation(addr_t, addr_t &, Allocator &) + bool lookup_rw_translation(addr_t, addr_t &, Allocator &) { Genode::raw(__func__, " not implemented yet"); return false; diff --git a/repos/base-hw/src/include/hw/spec/x86_64/page_table.h b/repos/base-hw/src/include/hw/spec/x86_64/page_table.h index 44bc24e67b..81d194d5e6 100644 --- a/repos/base-hw/src/include/hw/spec/x86_64/page_table.h +++ b/repos/base-hw/src/include/hw/spec/x86_64/page_table.h @@ -680,7 +680,7 @@ class Hw::Pml4_table _range_op(vo, 0, size, Remove_func(alloc)); } - bool lookup_translation(addr_t const, addr_t &, Allocator &) + bool lookup_rw_translation(addr_t const, addr_t &, Allocator &) { Genode::raw(__func__, " not implemented yet"); return false;