Re: [RFC 5/5] xen/x86: livepatch: enable attaching callbacks

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Hi Ryo,

Thanks for including x86 support alongside Arm64. I'd like to help
review this part too, although I'm still getting familiar with these
paths and have used LLM assistance. Could you help me check a few
interactions with tracing itself and the existing x86 conventions?

> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -365,6 +369,35 @@ int arch_livepatch_secure(const void *va, unsigned int 
> pages, enum va_type type)
> +#ifdef CONFIG_X86_PATCHABLE_FUNCTION_ENTRY
> +unsigned long arch_livepatch_trace_entry(unsigned long addr)
> +{
> +    if ( (is_endbr64((const void *)addr) || is_endbr64_poison((const void 
> *)addr)) )
> +        addr += ENDBR64_LEN;
> +
> +    return addr;
> +}

Could this helper itself be selected as a tracing target? It appears
to be an ordinary function in an object compiled with patchable
entries, and dispatch now calls it before reaching the callback:

> --- a/xen/common/hypfs_trace.c
> +++ b/xen/common/hypfs_trace.c
> @@ -141,7 +141,7 @@ void trace_dispatch_callbacks(unsigned long ip, unsigned 
> long parent_ip)
> -    if ( (unsigned long)global_trace_func.old_addr != ip )
> +    if ( arch_livepatch_trace_entry((unsigned 
> long)global_trace_func.old_addr) != ip )
>          return;
>
>      callback = global_trace_func.new_addr;

If the helper is the selected target and recording is enabled, could
this call enter the trampoline and dispatcher again, which then calls
the helper again before the recorder's recursion guard is reached?
What would contain that recursion before Xen runs out of stack?

Would excluding the helper from instrumentation on both architectures
help, together with checking the rest of the pre-callback dispatch
path? The Arm64 helper introduced here looks relevant too:

> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -497,6 +497,11 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> *elf,
>  #ifdef CONFIG_ARM64_PATCHABLE_FUNCTION_ENTRY
> +unsigned long arch_livepatch_trace_entry(unsigned long addr)
> +{
> +    return addr;
> +}

To illustrate, I think the helper's compiler-generated tracing entry
could be excluded with a small change like:

--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -497,7 +497,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
 }
 
 #ifdef CONFIG_ARM64_PATCHABLE_FUNCTION_ENTRY
-unsigned long arch_livepatch_trace_entry(unsigned long addr)
+unsigned long __attribute__((patchable_function_entry(0, 0)))
+arch_livepatch_trace_entry(unsigned long addr)
 {
     return addr;
 }
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -370,7 +370,8 @@ int arch_livepatch_secure(const void *va, unsigned int 
pages, enum va_type type)
 }
 
 #ifdef CONFIG_X86_PATCHABLE_FUNCTION_ENTRY
-unsigned long arch_livepatch_trace_entry(unsigned long addr)
+unsigned long __attribute__((patchable_function_entry(0, 0)))
+arch_livepatch_trace_entry(unsigned long addr)
 {
     if ( (is_endbr64((const void *)addr) || is_endbr64_poison((const void 
*)addr)) )
         addr += ENDBR64_LEN;

Would this remove these helpers from the compiler-generated entry
table, using the same attribute as the dispatcher? It only excludes
these two functions from compiler instrumentation; it does not provide
a general recursion guard or audit their callees. Uploaded trace
payloads would still need trace-site validation to enforce the same
exclusions, rather than relying only on HypFS's entry-table check.

Separately, does the new trampoline need the mitigation-aware RET
abstraction used elsewhere in Xen?

> --- /dev/null
> +++ b/xen/arch/x86/livepatch-asm.S
> @@ -0,0 +1,60 @@
> +        popq    %rax
> +        popfq
> +        popq    %rbp
> +        ret
> +END(xen_livepatch_trace_caller)

For reference, the existing asm/asm-defns.h, included through
asm/asm_defns.h, contains:

> #ifdef CONFIG_RETURN_THUNK
> # define RET jmp __x86_return_thunk
> #else
> # define RET ret
> #endif

With CONFIG_RETURN_THUNK enabled, wouldn't a literal ret here bypass
the configured return thunk? To illustrate, I think this could be
addressed with a small change like:

--- a/xen/arch/x86/livepatch-asm.S
+++ b/xen/arch/x86/livepatch-asm.S
@@ -56,5 +56,5 @@ FUNC(xen_livepatch_trace_caller)
         popq    %rax
         popfq
         popq    %rbp
-        ret
+        RET
 END(xen_livepatch_trace_caller)

Would this preserve the configured return convention with the existing
include chain?

I also wondered about the C stack alignment at the dispatcher call:

> --- /dev/null
> +++ b/xen/arch/x86/livepatch-asm.S
> @@ -0,0 +1,60 @@
> +FUNC(xen_livepatch_trace_caller)
> +        pushq   %rbp
> +        movq    %rsp, %rbp
> +        pushfq
> +
> +        /* Preserve general-purpose registers. */
> +        pushq   %rax
> +        pushq   %rcx
> +        pushq   %rdx
> +        pushq   %rsi
> +        pushq   %rdi
> +        pushq   %r8
> +        pushq   %r9
> +        pushq   %r10
> +        pushq   %r11
> [...]
> +        movq    8(%rbp), %rdi
> +        subq    $5, %rdi
> +        movq    16(%rbp), %rsi
> +        call    xen_livepatch_trace_dispatcher

For a build using the normal 16-byte C stack boundary, wouldn't the
traced function enter with RSP eight modulo sixteen? The injected call
then makes it zero, and these eleven pushes leave it eight immediately
before calling C, rather than zero. Would that give the dispatcher the
wrong incoming alignment in such a build?

The existing xen/arch/x86/arch.mk makes the reduced boundary conditional:

> # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
> # this to be overridden elsewhere.
> $(call cc-option-add,CFLAGS_stack_boundary,CC,-mpreferred-stack-boundary=3)

Could the trampoline establish the dispatcher's required alignment
without assuming this reduction is active, and restore the exact
original stack afterwards? Is there another build constraint which
rules out the 16-byte case for this feature?

Finally, could the x86 entry checks address the same restoration issue
as the Arm64 path discussed on patch 1?

> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -365,6 +369,35 @@ int arch_livepatch_secure(const void *va, unsigned int 
> pages, enum va_type type)
> +void noinline arch_livepatch_apply_trace(const struct livepatch_func *trace)
> +{
> +    void *site = (void *)arch_livepatch_trace_entry((unsigned 
> long)trace->old_addr);
> +    const void *new_addr = xen_livepatch_trace_caller;
> +    uint8_t insn[ARCH_PATCH_INSN_SIZE];
> +    int32_t val = new_addr - (site + ARCH_PATCH_INSN_SIZE);
> +
> +    insn[0] = 0xe8; /* CALL rel32. */
> +    memcpy(&insn[1], &val, sizeof(val));
> +    memcpy(site, insn, sizeof(insn));
> +}
> +
> +void noinline arch_livepatch_revert_trace(const struct livepatch_func *trace)
> +{
> +    void *site = (void *)arch_livepatch_trace_entry((unsigned 
> long)trace->old_addr);
> +
> +    memset(site, 0x90, ARCH_PATCH_INSN_SIZE);
> +}

The existing arch_livepatch_verify_func() in this file checks the
replacement space like this; these lines are not added by this patch:

>         uint8_t needed = ARCH_PATCH_INSN_SIZE;
>
>         if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) )
>             needed += ENDBR64_LEN;
>
>         if ( func->old_size < needed )
>             return -EINVAL;

ARCH_PATCH_INSN_SIZE is five in asm/livepatch.h. Am I right that this
checks space but not whether the original bytes are reserved tracing
NOPs? Could an uploaded .livepatch.traces payload name a sufficiently
large uninstrumented entry, have real instructions overwritten, and
then lose them when revert substitutes five NOPs?

Would shared trace-site validation for uploaded payloads and HypFS
requests help establish the original bytes and reject overlapping
ordinary livepatches? Could the applied offset and restoration state
also be retained rather than inferred again from a potentially changed
entry when reverting?

Thanks for taking a look. I may have missed a guard or architecture
constraint elsewhere, so I'd appreciate your view on these cases.

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>