Re: [Xen-devel] [PATCH] x86/emul: Correct the decoding of mov to/from cr/dr

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.03.17 at 14:30, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/03/17 13:10, Jan Beulich wrote:
>>>>> On 06.03.17 at 11:30, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> The mov to/from cr/dr behave as if they were encoded with Mod = 3.  When
>>> encoded with Mod != 3, no displacement or SIB bytes are fetched.
>> Would mind letting us know how you became aware of this oddity?
>> It's clearly both unexpected
> 
> Most definitely.
> 
>> and undocumented,
> 
> Actually, the contrary.  It is explicitly called out in both Intel and
> AMDs instruction manual, which is why I noticed it.

Ah, well hidden in the middle of text. Using standard opcode
representation for non-standard encodings is far from helpful -
that's what I look at mainly (and the text becomes important
only when there _are_ oddities there).

> I have confirmed that hardware doesn't fetch disp or SIB bytes on my
> oldest and newest Intel and AMD boxes.

So did I.

>> and I wonder whether there are any other opcodes behaving the same.
> 
> Not that I have spotted.

Neither me, but there must be something in the decoder special
casing those insns, and it's easily imaginable for such special cases
to also apply elsewhere.

>>> --- a/tools/tests/x86_emulator/test_x86_emulator.c
>>> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
>>> @@ -894,6 +894,27 @@ int main(int argc, char **argv)
>>>      }
>>>      printf("okay\n");
>>>  
>>> +    printf("%-40s", "Testing mov %%cr4,%%esi (bad ModRM)...");
>>> +    /*
>>> +     * Mod = 1, Reg = 4, R/M = 6 would normally encode a memory reference 
>>> of
>>> +     * disp8(%esi), but mov to/from cr/dr are special and behave as if they
>>> +     * were encoded with Mod == 3.
>>> +     */
>>> +    instr[0] = 0x0f; instr[1] = 0x20, instr[2] = 0146;
>> I can guess why you've done it this way, but I'd prefer if we didn't
>> start using octal numbers. I for one am very used to reading ModRM
>> bytes in their hex representation.
> 
> The argument ought to be for which representation is easier.  I'd accept
> an argument for consistency with the rest of the code, but are you
> seriously saying you think that hex is easier to read/manipulate than
> octal for ModRM/SIB bytes?

Yes, and very seriously in fact. Nevertheless I admit that this is a
matter of what one is used to. The only alternative to hex I'd
freely accept would be to use bit fields to fill the pieces individually.

>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -2086,7 +2086,8 @@ x86_decode_twobyte(
>>>          }
>>>          /* fall through */
>>>      case 0x21: case 0x23: /* mov to/from dr */
>>> -        generate_exception_if(lock_prefix || ea.type != OP_REG, EXC_UD);
>>> +        ASSERT(ea.type == OP_REG); /* Early operand adjustment ensures 
>>> this. */
>>> +        generate_exception_if(lock_prefix, EXC_UD);
>>>          op_bytes = mode_64bit() ? 8 : 4;
>>>          break;
>>>  
>>> @@ -2427,6 +2428,23 @@ x86_decode(
>>>                  break;
>>>              }
>>>          }
>>> +        else if ( ext == ext_0f )
>>> +        {
>>> +            switch ( b )
>>> +            {
>>> +            case 0x20: /* mov cr,reg */
>>> +            case 0x21: /* mov dr,reg */
>>> +            case 0x22: /* mov reg,cr */
>>> +            case 0x23: /* mov reg,dr */
>>> +                /*
>>> +                 * Mov to/from cr/dr ignore the encoding of Mod, and 
>>> behave as
>>> +                 * if they were encoded as reg/reg instructions.  No futher
>>> +                 * disp/SIB bytes are fetched.
>>> +                 */
>>> +                modrm_mod = 3;
>>> +                break;
>>> +            }
>>> +        }
>> Just like done by
>> https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg00588.html 
>> this calls for the outer if/else to be morphed into a switch().
> 
> I did that first, but it was rather complicated to read.  I can switch
> back to that if you'd prefer.

Well, if you don't here then that other patch will in any case.

Jan

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