On Mon, Nov 27, 2023 at 10:48:29AM -0600, Madhavan T. Venkataraman wrote: > Apologies for the late reply. I was on vacation. Please see my response below: > > On 11/13/23 02:19, Peter Zijlstra wrote: > > On Sun, Nov 12, 2023 at 09:23:24PM -0500, Mickaël Salaün wrote: > >> From: Madhavan T. Venkataraman <madve...@linux.microsoft.com> > >> > >> X86 uses a function called __text_poke() to modify executable code. This > >> patching function is used by many features such as KProbes and FTrace. > >> > >> Update the permissions counters for the text page so that write > >> permissions can be temporarily established in the EPT to modify the > >> instructions in that page. > >> > >> Cc: Borislav Petkov <b...@alien8.de> > >> Cc: Dave Hansen <dave.han...@linux.intel.com> > >> Cc: H. Peter Anvin <h...@zytor.com> > >> Cc: Ingo Molnar <mi...@redhat.com> > >> Cc: Kees Cook <keesc...@chromium.org> > >> Cc: Madhavan T. Venkataraman <madve...@linux.microsoft.com> > >> Cc: Mickaël Salaün <m...@digikod.net> > >> Cc: Paolo Bonzini <pbonz...@redhat.com> > >> Cc: Sean Christopherson <sea...@google.com> > >> Cc: Thomas Gleixner <t...@linutronix.de> > >> Cc: Vitaly Kuznetsov <vkuzn...@redhat.com> > >> Cc: Wanpeng Li <wanpen...@tencent.com> > >> Signed-off-by: Madhavan T. Venkataraman <madve...@linux.microsoft.com> > >> --- > >> > >> Changes since v1: > >> * New patch > >> --- > >> arch/x86/kernel/alternative.c | 5 ++++ > >> arch/x86/mm/heki.c | 49 +++++++++++++++++++++++++++++++++++ > >> include/linux/heki.h | 14 ++++++++++ > >> 3 files changed, 68 insertions(+) > >> > >> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > >> index 517ee01503be..64fd8757ba5c 100644 > >> --- a/arch/x86/kernel/alternative.c > >> +++ b/arch/x86/kernel/alternative.c > >> @@ -18,6 +18,7 @@ > >> #include <linux/mmu_context.h> > >> #include <linux/bsearch.h> > >> #include <linux/sync_core.h> > >> +#include <linux/heki.h> > >> #include <asm/text-patching.h> > >> #include <asm/alternative.h> > >> #include <asm/sections.h> > >> @@ -1801,6 +1802,7 @@ static void *__text_poke(text_poke_f func, void > >> *addr, const void *src, size_t l > >> */ > >> pgprot = __pgprot(pgprot_val(PAGE_KERNEL) & ~_PAGE_GLOBAL); > >> > >> + heki_text_poke_start(pages, cross_page_boundary ? 2 : 1, pgprot); > >> /* > >> * The lock is not really needed, but this allows to avoid open-coding. > >> */ > >> @@ -1865,7 +1867,10 @@ static void *__text_poke(text_poke_f func, void > >> *addr, const void *src, size_t l > >> } > >> > >> local_irq_restore(flags); > >> + > >> pte_unmap_unlock(ptep, ptl); > >> + heki_text_poke_end(pages, cross_page_boundary ? 2 : 1, pgprot); > >> + > >> return addr; > >> } > > > > This makes no sense, we already use a custom CR3 with userspace alias > > for the actual pages to write to, why are you then frobbing permissions > > on that *again* ? > > Today, the permissions for a guest page in the extended page table > (EPT) are RWX (unless permissions are restricted for some specific > reason like for shadow page table pages). In this Heki feature, we > don't allow RWX by default in the EPT. We only allow those permissions > in the EPT that the guest page actually needs. E.g., for a text page, > it is R_X in both the guest page table and the EPT.
To what end? If you always mirror what the guest does, you've not actually gained anything. > For text patching, the above code establishes an alternate mapping in > the guest page table that is RW_ so that the text can be patched. That > needs to be reflected in the EPT so that the EPT permissions will > change from R_X to RWX. In other words, RWX is allowed only as > necessary. At the end of patching, the EPT permissions are restored to > R_X. > > Does that address your comment? No, if you want to mirror the native PTEs why don't you hook into the paravirt page-table muck and get all that for free? Also, this is the user range, are you saying you're also playing these daft games with user maps?
