Re: [PATCH 5/6] x86/alternative: Relocate all insn-relative fields
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
Re: [PATCH 5/6] x86/alternative: Relocate all insn-relative fields
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
[PATCH 5/6] x86/alternative: Relocate all insn-relative fields
Right now, relocation of displacements is restricted to finding 0xe8/e9 as the
first byte of the replacement, but this is overly restrictive.
Use x86_decode_lite() to find and adjust all insn-relative fields.
As with disp8's not leaving the replacemnet block, some disp32's don't either.
e.g. the RSB stuffing loop. These stay unmodified.
For now, leave the altcall devirtualisation alone. These require more care to
transform into the new scheme.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
---
xen/arch/x86/alternative.c | 46 +++---
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index c86ea235e865..4d7dc9418cf0 100644
--- 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;
+}
+
+if ( res.rel_type != REL_TYPE_d32 )
+continue;
+
+d32 = res.rel;
+
/*
* Detect the special case of indirect-to-direct branch
patching:
* - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9;
already
@@ -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();
-else
-*(int32_t *)(buf + 1) += repl - orig;
+
+target = ip + res.len + *d32;
+
+if ( target >= buf && target <= end )
+{
+/*
+ * Target doesn't leave the replacement block. e.g. RSB
+ * stuffing. Leave it unmodified.
+ */
+continue;
+}
+
+*d32 += repl - orig;
}
-else if ( force && system_state < SYS_STATE_active )
-ASSERT_UNREACHABLE();
}
a->priv = 1;
--
2.30.2
