Re: [Xen-devel] [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.17 at 19:45, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/11/17 08:32, Jan Beulich wrote:
>>>>> On 26.10.17 at 19:03, <euan.harris@xxxxxxxxxx> wrote:
>>> * invvpid has a 128-bit memory operand but we only require the VPID value
>>>   which lies in the lower 64 bits.
>> The memory operand (wrongly) isn't being read at all - I don't
>> understand the above bullet point for that reason.
>>
>>> @@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>>      unsigned long base, index, seg_base, disp, offset;
>>>      int scale, size;
>>>  
>>> +    unsigned int bytes = vmx_guest_x86_mode(v);
>>> +
>> Except in extraordinary circumstances please don't add blank lines in
>> the middle of declarations.
>>
>> Also - don't you get 2 here for 16-bit protected mode, which you'd
>> need to convert to 4?
> 
> We can never reach this point from a 16 bit segment.  All VMX
> instructions #UD if executed outside of a 64bit (in long mode) or 32bit
> (outside of long mode) segment.

That's certainly not what the insn pages say: The common conditional
used is

IF (not in VMX operation) or (CR0.PE = 0) or (RFLAGS.VM = 1) or (IA32_EFER.LMA 
= 1 and CS.L = 0)
THEN #UD;

which excludes real, VM86, and compat modes, but not 16-bit
protected mode. Of course there's the option of hardware
behaving like you say and the manual just being wrong.

>>> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>>>      {
>>>      case INVEPT_SINGLE_CONTEXT:
>>>      {
>>> -        unsigned long eptp;
>>> +        uint64_t eptp;
>>>  
>>> -        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
>>> +        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));
>> I think you should read the full 128 bits for correct faulting behavior
>> (also for invvpid). I also can't derive from the SDM that this reading
>> won't occur for the INVEPT_ALL_CONTEXT case. Sadly the SDM
>> doesn't say whether the reserved upper half is enforced to be zero
>> (which we would then need to emulate), or whether it is being
>> ignored. For invvpid however it does state that bits 16:63 are being
>> checked.
> 
> Observations on real hardware to the contrary.  Each of the generations
> I've tried never read the operand, or the part of the operand that they
> don't need.

Correct faulting behavior implies more than whether the actual read
occurs: A quick test confirms that the ept_sync_all() used during
VMX enabling ends in #GP when the memory address of INVEPT is
being changed to a non-canonical one, and in #PF if changed to a
suitable canonical one (on a Westmere system).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel