Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 30, 2022 at 01:05:31PM +0200, Jan Beulich wrote:
> While not triggered by the trivial xen_nop in-tree patch on
> staging/master, that patch exposes a problem on the stable trees, where
> all functions have ENDBR inserted. When NOP-ing out a range, we need to
> account for this. Handle this right in livepatch_insn_len().
> 
> This requires livepatch_insn_len() to be called _after_ ->patch_offset
> was set. Note however that the earlier call cannot be deleted. In fact
> its result should have been used to guard the is_endbr64() /
> is_endbr64_poison() invocations - add the missing check now. While
> making that adjustment, also use the local variable "old_ptr"
> consistently.
> 
> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced 
> functions")

I have to admit I'm confused as to why that commit carries a Tested-by
from Arm.  Did Arm test the commit on x86 hardware?  Because that
commit only touches x86 specific code.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

FWIW, on the original implementation, I think it would have been
clearer to advance old_ptr and adjust the length?

> ---
> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
> ---
> Only build tested, as I don't have a live patching environment available.
> 
> For Arm this assumes that the patch_offset field starts out as zero; I
> think we can make such an assumption, yet otoh on x86 explicit
> initialization was added by the cited commit.
> 
> Note that the other use of is_endbr64() / is_endbr64_poison() in
> arch_livepatch_verify_func() would need the extra check only for
> cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
> Hence I'm not altering the code there.
> 
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>       * ENDBR64 or similar instructions).
>       */
> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
> +    if ( len >= ENDBR64_LEN &&
> +         (is_endbr64(old_ptr) || is_endbr64_poison(old_ptr)) )
>          func->patch_offset += ENDBR64_LEN;
>  
> +    /* This call must be re-issued once ->patch_offset has its final value. 
> */
> +    len = livepatch_insn_len(func);
> +    if ( !len )
> +        return;
> +
>      memcpy(func->opaque, old_ptr + func->patch_offset, len);
>      if ( func->new_addr )
>      {
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -90,7 +90,7 @@ static inline
>  unsigned int livepatch_insn_len(const struct livepatch_func *func)
>  {
>      if ( !func->new_addr )
> -        return func->new_size;
> +        return func->new_size - func->patch_offset;

Seeing as func->patch_offset is explicitly initialized in
arch_livepatch_apply for x86, do we also need to do the same on Arm
now that the field will be used by common code?

Maybe the initialization done in arch_livepatch_apply for x86 is not
strictly required.

Thanks, Roger.