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

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Hi Ryo,

Thanks for posting this RFC. Having dynamic function tracing in Xen
looks useful, and I'd like to help with reviews. I'm not yet deeply
familiar with this part of the code, so I've used LLM assistance to
help investigate it. I may have missed a check elsewhere;
could you help me confirm whether the following concerns apply?

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

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?

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?

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?

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?

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

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

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>