Re: [RFC PATCH v2 17/19] heki: x86: Update permissions counters during text patching

["Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>]

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 <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> 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 <bp@xxxxxxxxx>
>> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
>> Cc: Mickaël Salaün <mic@xxxxxxxxxxx>
>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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.

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?

Madhavan