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.
Jan
