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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/03/16 16:21, Jan Beulich wrote:
>>>> On 07.03.16 at 17:11, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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
> Well, such missing further fixup would be noticed immediately by
> someone trying (unless the patch code path never gets executed).
> Whereas a simply adjustment to register state would seem quite
> reasonable to follow a call. While right now the subsequent
> patches don't depend on this being >= or ==, I think it was wrong
> to be == from the beginning.
>
> Plus - there are endless other possibilities of instructions needing
> fixups (most notably such with RIP-relative memory operands),
> none of which are even remotely reasonable to deal with here.
> I.e. namely in the absence of a CALL/JMP the same issue would
> exist anyway, which is why I'm not overly concerned of those.
> All we want is a specific special case to be treated correctly.

Fair enough.  Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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