Re: [PATCH] x86_emulate: Always truncate %eip out of long mode

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/01/16 15:44, Jan Beulich wrote:
> Ping?

Sorry - this is still on my todo list, but I have more urgent work
currently.

~Andrew

>
>>>> On 15.12.15 at 09:53, <JBeulich@suse.com> wrote:
>>>>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -570,8 +570,10 @@ do{ asm volatile (                                       
>>>                \
>>>  /* Fetch next part of the instruction being emulated. */
>>>  #define insn_fetch_bytes(_size)                                         \
>>>  ({ unsigned long _x = 0, _eip = _regs.eip;                              \
>>> -   if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \
>>> -   _regs.eip += (_size); /* real hardware doesn't truncate */           \
>>> +   _regs.eip += (_size);                                                \
>>> +   if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */  \
>>> +       _eip      &= ~((1UL << (def_ad_bytes * 8)) - 1);                 \
>>> +       _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); };              \
>> Did you test this? The ~ seems quite wrong, and this would also
>> cause undefined behavior in 32-bit compilation of the emulator
>> test... Anyway, how about the following replacement patch,
>> along the lines of what I think I had described in reply to the first
>> variant of the patch?
>>
>> Jan
>>
>> x86emul: fix rIP handling
>>
>> Deal with rIP just like with any other register: Truncate to designated
>> width upon entry, and write back the zero-extended 32-bit value when
>> emulating 32-bit code, and leave the upper 48 bits unchanged for 16-bit
>> code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -570,7 +570,6 @@ do{ asm volatile (
>>  /* Fetch next part of the instruction being emulated. */
>>  #define insn_fetch_bytes(_size)                                         \
>>  ({ unsigned long _x = 0, _eip = _regs.eip;                              \
>> -   if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \
>>     _regs.eip += (_size); /* real hardware doesn't truncate */           \
>>     generate_exception_if((uint8_t)(_regs.eip -                          \
>>                                     ctxt->regs->eip) > MAX_INST_LEN,     \
>> @@ -1481,6 +1480,10 @@ x86_emulate(
>>  #endif
>>      }
>>  
>> +    /* Truncate rIP to def_ad_bytes (2 or 4) if necessary. */
>> +    if ( def_ad_bytes < sizeof(_regs.eip) )
>> +        _regs.eip &= (1UL << (def_ad_bytes * 8)) - 1;
>> +
>>      /* Prefix bytes. */
>>      for ( ; ; )
>>      {
>> @@ -3793,6 +3796,21 @@ x86_emulate(
>>  
>>      /* Commit shadow register state. */
>>      _regs.eflags &= ~EFLG_RF;
>> +    switch ( __builtin_expect(def_ad_bytes, sizeof(_regs.eip)) )
>> +    {
>> +        uint16_t ip;
>> +
>> +    case 2:
>> +        ip = _regs.eip;
>> +        _regs.eip = ctxt->regs->eip;
>> +        *(uint16_t *)&_regs.eip = ip;
>> +        break;
>> +#ifdef __x86_64__
>> +    case 4:
>> +        _regs.rip = _regs._eip;
>> +        break;
>> +#endif
>> +    }
>>      *ctxt->regs = _regs;
>>  
>>   done:
>
>