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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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