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

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

On Sat, 24 Oct 2026 16:36:30, Bernhard Kaindl wrote:
>> --- 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:

Yes, it could.
I missed the function to mark uninstrumentable.

>> --- 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?

Yes, it could enter recursion.

>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?

Thank you. Yes, I think this should work.
I'll make sure to mark both functions uninstrumentable.

>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.

Understood.

For the former concern on recursion, I'll put a guard covering the dispatcher
following your suggestion:
https://lore.kernel.org/xen-devel/179284608595.1706038.12585790247811677658.b4-review@b4/

For the latter trace-site validation, please see arch_livepatch_verify_entry()
which I propose in the reply to your 1/5 patch feedback:
https://lore.kernel.org/xen-devel/20261026073022.110170-1-takakura@xxxxxxxxxxxxx/
I'll add the corresponding x86 validation.

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

I see.
Yes, I think it needs to be RET.

>> --- /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?

Yes, I'll fix this in the new version.

>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?

I see.
I checked it and was working on 8 bytes aligned stack.

>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?

There is no such constraint ruling out the 16-byte case right now.
And I'm thinking of making the trampoline into C function with
the attribute force_align_arg_pointer to make sure the stack is aligned.
Do you think this would work?

About the discussion on making the trampoline into C function,
please see the Andrew's feedback:
https://lore.kernel.org/xen-devel/97d772f1-322f-43f6-a793-fdbba16c7f04@xxxxxxxxxx/

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

I see. Yes, I think there is the same issue.

>> --- 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?

Yes, that could happen.

>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?

I see, I agree with both.

For the former, I believe it's addressed by my reply to your
patch 1/5 feedback. Please take a look at arch_livepatch_verify_entry().
https://lore.kernel.org/xen-devel/20261026073022.110170-1-takakura@xxxxxxxxxxxxx/

For the latter, I'll follow the existing arch_livepatch_apply()/revert()'s
save/restore routine in the next version.

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

No, you didn't miss anything. Thank you for pointing them out.

Sincerely,
Ryo Takakura