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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/04/2024 6:54 am, Jan Beulich wrote:
> On 23.04.2024 16:59, Jan Beulich wrote:
>> 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.
> Actually, after some further thought, I don't even think we should skip
> such alternatives. Think of e.g. cases where in principle we could get
> away with just patching the prefix of an insn. Yet even without such
> trickery - there's a fair chance that the alternative doesn't need
> fiddling with, and hence putting it in unaltered is likely the best we
> can do here.

Following Roger's series, it needs to be a `return -Exx` and non-fatal
in livepatch context.

That said, the point of the SELF_TESTS, and the userspace harness I
didn't finish for v1, is to avoid (as far as possible) getting into the
situation where we can't decode the replacements.

~Andrew