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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

> and I wonder whether there are any other opcodes behaving the same.

Not that I have spotted.

> In any even I think we want to make at least an attempt at having HW
> vendors confirm this (and to address the other-opcodes concern),
> so I'm extending the Cc list.
>
>> --- 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?

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

~Andrew

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