Re: [PATCH v5 2/2] xen/x86: Livepatch: support patching CET-enhanced functions

[Jan Beulich <jbeulich@suse.com>]

On 09.03.2022 15:53, Bjoern Doebel wrote:
> Changes since r1:
> * use sizeof_field() to avoid unused variable warning
> * make metadata variable const in arch_livepatch_revert
> * rebase on top and make use of Andrew Cooper's was_endbr64() patch
> * use padding byte to store offset rather than reducing opaque area

You seem to accumulate things here, thus making it impossible to spot
what was changed from the previous version. Retaining all changes
information is helpful, but it wants splitting up suitably.

It would also have been helpful if you had mentioned the 2nd
was_endbr64() that has appeared now.

I'm glad to see the casts are gone now, thanks.

> @@ -114,8 +115,21 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
>          if ( func->old_size < func->new_size )
>              return -EINVAL;
>      }
> -    else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> -        return -EINVAL;
> +    else
> +    {
> +        /*
> +         * Space needed now depends on whether the target function
> +         * start{s,ed} with an ENDBR64 instruction.
> +         */
> +        uint8_t needed;
> +
> +        needed = ARCH_PATCH_INSN_SIZE;

Surely this can be the initializer of the variable?

> @@ -130,12 +144,24 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
>      uint8_t insn[sizeof(func->opaque)];
>      unsigned int len;
>  
> +    func->patch_offset = 0;
>      old_ptr = func->old_addr;
>      len = livepatch_insn_len(func);
>      if ( !len )
>          return;
>  
> -    memcpy(func->opaque, old_ptr, len);
> +    /*
> +    * CET hotpatching support: We may have functions starting with an ENDBR64
> +    * instruction that MUST remain the first instruction of the function, hence
> +    * we need to move any hotpatch trampoline further into the function. For that
> +    * we need to keep track of the patching offset used for any loaded hotpatch
> +    * (to avoid racing against other fixups adding/removing ENDBR64 or similar
> +    * instructions).
> +    */

Bad indentation of all but the first line of this comment. Also the
middle on of the lines is too long.

> +    if ( is_endbr64(old_ptr)  || was_endbr64(func->old_addr) )

Stray double blank in the middle.

> @@ -143,14 +169,15 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
>          BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
>  
>          insn[0] = 0xe9; /* Relative jump. */
> -        val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +        val = func->new_addr - (func->old_addr + func->patch_offset
> +                                + ARCH_PATCH_INSN_SIZE);

Nit: On split lines the operator still goes on the previous line. (We
often make an exception for ?:, but not normally anything else.)

> @@ -159,7 +186,7 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
>   */
>  void noinline arch_livepatch_revert(const struct livepatch_func *func)
>  {
> -    memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
> +    memcpy(func->old_addr + func->patch_offset, func->opaque, livepatch_insn_len(func));

This line is now too long.

Jan