Re: [Xen-devel] [PATCH for-4.5 v8 4/7] xen: Add vmware_port support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.01.15 at 16:58, <dslutz@xxxxxxxxxxx> wrote:
> On 01/22/15 03:32, Jan Beulich wrote:
>>>>> On 21.01.15 at 18:52, <dslutz@xxxxxxxxxxx> wrote:
>>> On 01/16/15 05:09, Jan Beulich wrote:
>>>>>>> On 03.10.14 at 00:40, <dslutz@xxxxxxxxxxx> wrote:
>>>>> +
>>>>> +        /* Only adjust byte_cnt 1 time */
>>>>> +        if ( bytes[0] == 0x66 )     /* operand size prefix */
>>>>> +        {
>>>>> +            if ( byte_cnt == 4 )
>>>>> +                byte_cnt = 2;
>>>>> +            else
>>>>> +                byte_cnt = 4;
>>>>> +        }
>>>> Iirc REX.W set following 0x66 cancels the effect of the latter. Another
>>>> thing x86emul would be taking care of for you if you used it.
>>> I did not know this.  Most of my testing was done without any check
>>> for prefix(s).  I.E. (Open) VMware Tools only uses the inl.  I do
>>> not know of anybody using 16bit segments and VMware tools.
>> But this isn't the perspective to take when adding code to the
>> hypervisor - you should always consider what a (perhaps
>> malicious) guest could do.
> 
> Ok, but my read of this statement does not help decide which way
> to go.  I see several options:
> 
> 1) Only allow #GP to work for 0xed (inl (%dx),%eax).
>     Pros: No attack surface for malicious guest.
>              No need for get_instruction_length().
>              No need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     Cons: There may exist user apps. that work on VMware and
>                not on xen (16bit segments, realmode, vm86, etc).
> 
> 2) Only allow #GP to work for all 4 I/O instructions without prefix.
>     Pros: No attack surface for malicious guest.
>              No need for get_instruction_length().
>              No need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     Cons: There may exist user apps. that work on VMware and
>                  not on xen (16bit segments, realmode, vm86, etc).
> 
> 3) Only allow zero or one 0x66 prefix and 0xed (inl (%dx),%eax).
>     Pros: No attack surface for malicious guest.
>              No need for get_instruction_length().
>              No need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     Cons: There may exist user apps. that work on VMware and
>                  not on xen (using too many prefixes, using other
>                  opcodes).
> 
> 4) Only allow zero or one 0x66 prefix and all 4 I/O instructions.
>     Pros: No attack surface for malicious guest.
>              No need for get_instruction_length().
>              No need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     Cons: There may exist user apps. that work on VMware and
>                  not on xen (using too many prefixes).
> 
> 5) Only allow zero to 14 0x66 prefix and 0xed (inl (%dx),%eax).
>     Pros: No attack surface for malicious guest.
>     Cons: There may exist user apps. that work on VMware and
>                 not on xen (using unneeded prefixes, using other
>                 opcodes).
>     5a:    Would be cleaner with get_instruction_length() on Intel,
>                 but would need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     5b:     Always pass in MAX_INST_LEN.
>    
> 
> 6) Only allow zero to 14 0x66 prefix and all 4 I/O instructions.
>     Pros: No attack surface for malicious guest.
>     Cons: There may exist user apps. that work on VMware and
>                not on xen (using unneeded  prefixes).
>     6a:    Would be cleaner with get_instruction_length() on Intel,
>                 but would need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     6b:     Always pass in MAX_INST_LEN.
> 
> 7) Add complete prefix handling, and all 4 I/O instructions
>     Pros: Limited attack surface for malicious guest (the handling
>               of all prefixes greatly increases the complexity of the
>               code).
>     Cons: Lots of added code.
>     7a:    Would be cleaner with get_instruction_length() on Intel,
>                 but would need for Intel to confirm the necessary hardware
>                 behaviour as being architectural.
>     7b:     Always pass in MAX_INST_LEN.
> 
> 8) Use hvm_emulate_one().
>     Pros: shares code, reduces new code.
>     Cons: Adds a lot of attack surface for malicious guest.
> 
> 
> I had picked #6, you asked for #8, but I read your answer as do not
> do #8.

I don't think it can or should be read that way; in particular - without
having seen it to be the case in whatever code it takes - I don't buy
the argument of this adding a lot of attack surface: The emulator
code is there anyway.

> I would be happy to go with any of the 8 ways (or a way I did not list
> above),
> just need to know which one to focus on.

As stated before - if feasible, 8 would seem the best option. The
second best one would be to support all four I/O insns (assuming
VMware supports all of them too) with any legal (even if pointless
or redundant) prefix combination, and with the prefixes actually
doing something correctly emulated.

>>> So there are 3 options here:
>>> 1) Add an ASSERT() like the BUG_ON() in get_instruction_length()
>>> 2) Switch to using get_instruction_length()
>>> 3) Switch to using MAX_INST_LEN.
>>>
>>> Let me know which way to go.
>> As said above - use get_instruction_length() if Intel confirms the
>> necessary hardware behavior as being architectural. If they
>> don't, 3) looks like the only viable option.
> 
> 
> So what is the procedure to getting "Intel confirms the necessary hardware
> behaviour as being architectural"?

There's no procedure. Ask them explicitly (i.e. perhaps outside of
this thread, where the question may end up being well hidden from
their eyes), and then ping them until they give you a statement one
way or another.

Jan

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