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

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

Hi Bernhard!

Thank you for your feedback. I'm still going through your comments on
the other patches. I'll reply as I work through them.

On Sat, 24 Oct 2026 14:48:05 +0200 Bernhard Kaindl wrote:
>--- a/xen/arch/arm/arm64/livepatch.c
>+++ b/xen/arch/arm/arm64/livepatch.c
>@@ -492,6 +496,36 @@
>> +#ifdef CONFIG_ARM64_PATCHABLE_FUNCTION_ENTRY
>> +void arch_livepatch_apply_trace(const struct livepatch_func *trace)
>> +{
>> +    uint32_t *new_ptr;
>> +    uint32_t insns[2];
>> +
>> +    new_ptr = trace->old_addr - (void *)_start + vmap_of_xen_text;
>> [...]
>> +    memcpy(new_ptr, insns, sizeof(insns));
>
>This copies eight bytes from the two-element instruction array, while
>uploaded trace descriptors still use the ordinary Arm livepatch verifier?
>
>For reference, the existing arch_livepatch_verify_func() in
>xen/arch/arm/livepatch.c contains this size check (not added by this patch):
>
>>     if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>>         return -EINVAL;
>>
>>     return 0;

Thank you for pointing this out.
Yes, the check is insufficient.

>With ARCH_PATCH_INSN_SIZE equal to four bytes, doesn't this accept a
>four-byte function without establishing that two reserved tracing NOPs
>are present? Is there another check on this path that I have overlooked?

No, I didn't add any other check.

>Could an assembly entry or another uninstrumented function consequently
>have executable instructions overwritten? If reverting then installs
>two NOPs, could that lose the original instructions rather than restore
>them?

Yes, I think both could happen.

>Would it make sense to validate the complete reserved tracing entry,
>including the actual branch destination and any conflicting livepatch,
>before allowing the write? Could the expected entry also be checked at
>application time, so that a modification after upload is not missed?

Thanks you for the suggestion.

How about adding a helper on the upload/apply/revert paths
that checks if the function's entry is in the expected state?

---- BEGIN IDEA ----
/*
 * Make sure the patch site is in the expected state.
 *
 * patched = false: expect NOP + NOP.
 * patched = true:  expect MOV + BL.
 */
static int arch_livepatch_verify_entry(unsigned long addr,
                                            bool patched)
{
    const uint32_t *entry = (const uint32_t *)addr;
    uint32_t expected[2];
    uint32_t branch;

    /*
     * Assembly functions and C functions excluded from instrumentation
     * lack reserved NOPs.
     */
    if ( !is_patchable_function_entry(addr) )
        return -EINVAL;

    /*
     * Generate the expected BL instruction with its destination
     * set to the trampoline.
     */
    branch = aarch64_insn_gen_branch_imm(addr + ARCH_PATCH_INSN_SIZE,
                                         (unsigned 
long)xen_livepatch_trace_caller,
                                         AARCH64_INSN_BRANCH_LINK);

    /* Make sure the destination is within the branch range */
    if ( branch == AARCH64_BREAK_FAULT )
        return -EINVAL;

    if ( patched )
    {
        /*
         * Before reverting, make sure the site is as we patched:
         *  mov x9, lr
         *  bl  xen_livepatch_trace_caller
         */
        expected[0] = aarch64_insn_gen_move_reg(9, 30);
        expected[1] = branch;
    }
    else
    {
        /*
         * Before applying, make sure the site is as we reserved:
         *  nop
         *  nop
         */
        expected[0] = aarch64_insn_gen_nop();
        expected[1] = aarch64_insn_gen_nop();
    }

    /* Reject the (un)patching if the site is not as we expected */
    if ( entry[0] != expected[0] || entry[1] != expected[1] )
        return -EINVAL;

    return 0;
}
---- END IDEA ----

Note that is_patchable_function_entry() was introduced in patch 3/5
for checking whether the function at the given address is traceable.
I'll move the function to this patch.

>Separately, could a callback or one of its helpers call the traced
>function again before the first callback returns?
>
>> +  ASSERT(f->new_addr);
>> +  ((livepatch_trace_func_t)f->new_addr)((unsigned long)f->old_addr, 
>> parent_ip);
>
>What would contain a recursion before Xen runs out of stack? Would a
>guard covering the pre-callback dispatcher work as well as the callback
>itself help, or is there an existing restriction on callback targets
>that would rule this out?

There is no restriction preventing the recursion right now.

>I think marking the dispatcher itself non-patchable does not protect
>its callees or the callback's callees, so they should be protected too?

I see that marking the dispatcher alone is insufficient.
I'll add a guard that covers the dispatcher and callback following your 
suggestion.

>Thanks for taking a look, and apologies if I have missed an existing
>safeguard.

Thank you for pointing this out as well.

Sincerely,
Ryo Takakura