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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/03/16 15:56, Jan Beulich wrote:
>>>> On 07.03.16 at 16:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> The relaxation is solely because at least CALL could validly
> be followed by further instructions.

But without scanning the entire replacement buffer, there might be other
relocations needing to happen.

That would require decoding the instructions, which is an extreme faff. 
It would be better to leave it currently as-is to effectively disallow
mixing a jmp/call replacement with other code, to avoid the subtle
failure of a second relocation not taking effect

~Andrew

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