Re: [PATCH 5/6] x86/alternative: Relocate all insn-relative fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2024 20:14, Andrew Cooper wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -244,10 +244,31 @@ static void init_or_livepatch 
> _apply_alternatives(struct alt_instr *start,
>  
>          memcpy(buf, repl, a->repl_len);
>  
> +        /* Walk buf[] and adjust any insn-relative operands. */
> +        if ( a->repl_len )
>          {
> -            /* 0xe8/0xe9 are relative branches; fix the offset. */
> -            if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> +            uint8_t *ip = buf, *end = ip + a->repl_len;
> +
> +            for ( x86_decode_lite_t res; ip < end; ip += res.len )
>              {
> +                int32_t *d32;
> +                uint8_t *target;
> +
> +                res = x86_decode_lite(ip, end);
> +
> +                if ( res.len <= 0 )
> +                {
> +                    printk("Alternative for %ps [%*ph]\n",
> +                           ALT_ORIG_PTR(a), a->repl_len, repl);
> +                    printk("Unable to decode instruction in alternative - 
> ignoring.\n");
> +                    goto skip_this_alternative;

Can this really be just a log message? There are cases where patching has
to happen for things to operate correctly. Hence if not panic()ing, I'd
say we at least want to taint the hypervisor.

> @@ -317,14 +338,23 @@ static void init_or_livepatch 
> _apply_alternatives(struct alt_instr *start,
>                           */
>                          goto skip_this_alternative;
>                      }
> +
> +                    continue;
>                  }
> -                else if ( force && system_state < SYS_STATE_active )
> -                    ASSERT_UNREACHABLE();

This (and the other one below) is related to altcall patching, which you
say you mean to leave alone: During the 2nd pass, no un-processed CALL /
JMP should occur anymore that aren't altcall related.

Jan