| Message ID | 20240422181434.3463252-3-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: > In debug builds, walk all alternative replacements with x86_decode_lite(). > > This checks that we can decode all instructions, and also lets us check that > disp8's don't leave the replacement block. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> With pointed-to types consistently constified, technically: Reviewed-by: Jan Beulich <jbeulich@suse.com> One further suggestion and a question, though: > @@ -464,6 +465,54 @@ static void __init _alternative_instructions(bool force) > void __init alternative_instructions(void) > { > arch_init_ideal_nops(); > + > + /* > + * Walk all replacement instructions with x86_decode_lite(). This checks > + * both that we can decode all instructions within the replacement, and > + * that any near branch with a disp8 stays within the alternative itself. > + */ > + if ( IS_ENABLED(CONFIG_DEBUG) ) > + { > + struct alt_instr *a; > + > + for ( a = __alt_instructions; > + a < __alt_instructions_end; ++a ) > + { > + void *repl = ALT_REPL_PTR(a); > + void *ip = repl, *end = ip + a->repl_len; > + > + if ( !a->repl_len ) > + continue; > + > + for ( x86_decode_lite_t res; ip < end; ip += res.len ) > + { > + res = x86_decode_lite(ip, end); > + > + if ( res.len <= 0 ) > + { > + printk("Alternative for %ps [%*ph]\n", > + ALT_ORIG_PTR(a), a->repl_len, repl); > + panic("Unable to decode instruction at +%u in alternative\n", > + (unsigned int)(unsigned long)(ip - repl)); Instead of the double cast, maybe better use +%lu? And really we ought to have support for %tu, allowing the other cast t be dropped here, too. > + } > + > + if ( res.rel_type == REL_TYPE_d8 ) > + { > + int8_t *d8 = res.rel; > + void *target = ip + res.len + *d8; > + > + if ( target < repl || target > end ) > + { > + printk("Alternative for %ps [%*ph]\n", > + ALT_ORIG_PTR(a), a->repl_len, repl); > + panic("'JMP/Jcc disp8' at +%u leaves alternative block\n", > + (unsigned int)(unsigned long)(ip - repl)); > + } > + } Why's Disp8 more important to check than Disp32? A bad CALL in a replacement can't possibly be encoded with Disp8, and both JMP and Jcc are also more likely to be encoded with Disp32 when their target isn't in the same blob (but e.g. in a different section). Jan
On 23/04/2024 3:44 pm, Jan Beulich wrote: > On 22.04.2024 20:14, Andrew Cooper wrote: >> In debug builds, walk all alternative replacements with x86_decode_lite(). >> >> This checks that we can decode all instructions, and also lets us check that >> disp8's don't leave the replacement block. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > With pointed-to types consistently constified, technically: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. >> @@ -464,6 +465,54 @@ static void __init _alternative_instructions(bool force) >> void __init alternative_instructions(void) >> { >> arch_init_ideal_nops(); >> + >> + /* >> + * Walk all replacement instructions with x86_decode_lite(). This checks >> + * both that we can decode all instructions within the replacement, and >> + * that any near branch with a disp8 stays within the alternative itself. >> + */ >> + if ( IS_ENABLED(CONFIG_DEBUG) ) I've swapped this for SELF_TESTS now it exists. >> + } >> + >> + if ( res.rel_type == REL_TYPE_d8 ) >> + { >> + int8_t *d8 = res.rel; >> + void *target = ip + res.len + *d8; >> + >> + if ( target < repl || target > end ) >> + { >> + printk("Alternative for %ps [%*ph]\n", >> + ALT_ORIG_PTR(a), a->repl_len, repl); >> + panic("'JMP/Jcc disp8' at +%u leaves alternative block\n", >> + (unsigned int)(unsigned long)(ip - repl)); >> + } >> + } > Why's Disp8 more important to check than Disp32? A bad CALL in a > replacement can't possibly be encoded with Disp8, and both JMP and Jcc > are also more likely to be encoded with Disp32 when their target isn't > in the same blob (but e.g. in a different section). Whatever the likelihood of them existing, Disp8's cannot possibly be correct if they cross the boundary of the replacement. Checking for them has the side effect of running decode_lite() over all replacements. Disp32's do exist in both external and internal forms (retpoline), and the point of this series is to make all external cases usable. Therefore, there are no invalid cases. ~Andrew
On 25.09.2024 23:15, Andrew Cooper wrote: > On 23/04/2024 3:44 pm, Jan Beulich wrote: >> On 22.04.2024 20:14, Andrew Cooper wrote: >>> + if ( res.rel_type == REL_TYPE_d8 ) >>> + { >>> + int8_t *d8 = res.rel; >>> + void *target = ip + res.len + *d8; >>> + >>> + if ( target < repl || target > end ) >>> + { >>> + printk("Alternative for %ps [%*ph]\n", >>> + ALT_ORIG_PTR(a), a->repl_len, repl); >>> + panic("'JMP/Jcc disp8' at +%u leaves alternative block\n", >>> + (unsigned int)(unsigned long)(ip - repl)); >>> + } >>> + } >> Why's Disp8 more important to check than Disp32? A bad CALL in a >> replacement can't possibly be encoded with Disp8, and both JMP and Jcc >> are also more likely to be encoded with Disp32 when their target isn't >> in the same blob (but e.g. in a different section). > > Whatever the likelihood of them existing, Disp8's cannot possibly be > correct if they cross the boundary of the replacement. Checking for > them has the side effect of running decode_lite() over all replacements. > > Disp32's do exist in both external and internal forms (retpoline), and > the point of this series is to make all external cases usable. Okay, fine then. > Therefore, there are no invalid cases. There definitely are: Any pointing outside of the present replacement block, into another replacement block. Which can in principle happen if a label was used wrongly. Anything pointing outside the block really needs to be covered by logic adjusting the displacement when the alternative is being put in place. Jan
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index 2e7ba6e0b833..5bd256365def 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -15,6 +15,7 @@ #include <asm/traps.h> #include <asm/nmi.h> #include <asm/nops.h> +#include <asm/x86_emulate.h> #include <xen/livepatch.h> #define MAX_PATCH_LEN (255-1) @@ -464,6 +465,54 @@ static void __init _alternative_instructions(bool force) void __init alternative_instructions(void) { arch_init_ideal_nops(); + + /* + * Walk all replacement instructions with x86_decode_lite(). This checks + * both that we can decode all instructions within the replacement, and + * that any near branch with a disp8 stays within the alternative itself. + */ + if ( IS_ENABLED(CONFIG_DEBUG) ) + { + struct alt_instr *a; + + for ( a = __alt_instructions; + a < __alt_instructions_end; ++a ) + { + void *repl = ALT_REPL_PTR(a); + void *ip = repl, *end = ip + a->repl_len; + + if ( !a->repl_len ) + continue; + + for ( x86_decode_lite_t res; ip < end; ip += res.len ) + { + res = x86_decode_lite(ip, end); + + if ( res.len <= 0 ) + { + printk("Alternative for %ps [%*ph]\n", + ALT_ORIG_PTR(a), a->repl_len, repl); + panic("Unable to decode instruction at +%u in alternative\n", + (unsigned int)(unsigned long)(ip - repl)); + } + + if ( res.rel_type == REL_TYPE_d8 ) + { + int8_t *d8 = res.rel; + void *target = ip + res.len + *d8; + + if ( target < repl || target > end ) + { + printk("Alternative for %ps [%*ph]\n", + ALT_ORIG_PTR(a), a->repl_len, repl); + panic("'JMP/Jcc disp8' at +%u leaves alternative block\n", + (unsigned int)(unsigned long)(ip - repl)); + } + } + } + } + } + _alternative_instructions(false); }
In debug builds, walk all alternative replacements with x86_decode_lite(). This checks that we can decode all instructions, and also lets us check that disp8's don't leave the replacement block. 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 | 49 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)