Re: [PATCH 2/6] x86/alternative: Walk all replacements in debug builds

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

With pointed-to types consistently constified, technically:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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