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
