Re: [Xen-devel] [PATCH] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.10.16 at 11:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/10/16 10:16, Jan Beulich wrote:
>>>>> On 14.10.16 at 18:13, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -2980,8 +2964,8 @@ x86_emulate(
>>>          break;
>>>  
>>>      case 0x8c: /* mov Sreg,r/m */
>>> -        seg = decode_segment(modrm_reg);
>>> -        generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>>> +        seg = modrm_reg;
>> You need to mask with 7 here and ...
>>
>>> @@ -2992,8 +2976,8 @@ x86_emulate(
>>>          break;
>>>  
>>>      case 0x8e: /* mov r/m,Sreg */
>>> -        seg = decode_segment(modrm_reg);
>>> -        generate_exception_if(seg == decode_segment_failed, EXC_UD, -1);
>>> +        seg = modrm_reg;
>> ... here - I've just checked that at least Intel ignores REX.R.
> 
> Both points are covered by by the is_x86_user_segment() check which you
> have cropped out of context.

Very definitely not:

+        seg = modrm_reg;
+        generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1);

You raise an exception if REX.R is set, as that causes seg > x86_seg_gs.

> One option, if you would prefer, is to reverse the check and assignment,
> passing modrm_reg into is_x86_user_segment(), but I don't see any need
> for other code changes.

That wouldn't help at all.

>>> @@ -5520,4 +5504,15 @@ x86_insn_length(const struct x86_emulate_state 
>>> *state,
>>>      return state->eip - ctxt->regs->eip;
>>>  }
>>>  
>>> +static void __init __maybe_unused build_assertions(void)
>>> +{
>>> +    /* Check the values against SReg3 encoding in opcode/ModRM bytes. */
>>> +    BUILD_BUG_ON(x86_seg_es != 0);
>>> +    BUILD_BUG_ON(x86_seg_cs != 1);
>>> +    BUILD_BUG_ON(x86_seg_ss != 2);
>>> +    BUILD_BUG_ON(x86_seg_ds != 3);
>>> +    BUILD_BUG_ON(x86_seg_fs != 4);
>>> +    BUILD_BUG_ON(x86_seg_gs != 5);
>>> +}
>> If we really want this, I think it should be enabled for the test tool
>> too.
> 
> I wonder whether I can get away with having it as a static inline in
> x86_emulate.h

I think that would do.

Jan

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