Re: [Xen-devel] [PATCH 1/4] x86/alternatives: correct near branch check

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/03/16 11:27, Jan Beulich wrote:
> Make sure the near JMP/CALL check doesn't consume uninitialized
> data, not even in a benign way. And relax the length check at once.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -174,7 +174,7 @@ static void __init apply_alternatives(st
>          memcpy(insnbuf, replacement, a->replacementlen);
>  
>          /* 0xe8/0xe9 are relative branches; fix the offset. */
> -        if ( (*insnbuf & 0xfe) == 0xe8 && a->replacementlen == 5 )
> +        if ( a->replacementlen >= 5 && (*insnbuf & 0xfe) == 0xe8 )
>              *(s32 *)(insnbuf + 1) += replacement - instr;
>  
>          add_nops(insnbuf + a->replacementlen,
>
>
>

Swapping the order is definitely a good thing.

However, relaxing the length check seems less so.  `E8 rel32` or `E9
rel32` encodings are strictly 5 bytes long.

There are complications with the `67 E{8,9} rel16` encodings, but those
are not catered for anyway, and the manual warns about undefined
behaviour if used in long mode.

What is your usecase for relaxing the check?  IMO, if it isn't exactly 5
bytes long, there is some corruption somewhere and the relocation
should't happen.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel