| Message ID | 20240422181434.3463252-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/alternatives: Adjust all insn-relative fields | expand |
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
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
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
On 23/04/2024 3:59 pm, Jan Beulich wrote: > On 22.04.2024 20:14, Andrew Cooper wrote: >> @@ -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. Calling the parameter "force" was a mistake and I regret not asking for it to change. It should be a SEAL_ALTCALL flag of some form or another. But, that's not what this ASSERT() is doing. This assert is only checking that you don't try sealing a still-NULL altcall early, which is only doing nothing more than checking that the caller of _apply_alternatives() passed a parameter correctly. ~Andrew
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;
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 <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/alternative.c | 46 +++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 8 deletions(-)