Re: [PATCH v4 04/16] xen: Add is_vmware_port_enabled

["Slutz, Donald Christopher" <dslutz@verizon.com>]

On 09/19/14 11:50, Boris Ostrovsky wrote:
> On 09/19/2014 09:24 AM, Slutz, Donald Christopher wrote:
>> On 09/18/14 18:53, Boris Ostrovsky wrote:
>>> On 09/17/2014 02:23 PM, Slutz, Donald Christopher wrote:
>>>>
>>>> I just spent some time checking and found out that even with the cpu
>>>> reporting:
>>>>
>>>> (XEN)   - Next-RIP Saved on #VMEXIT
>>>>
>>>> svm_nextrip_insn_length(v) is 0.
>>> As I see it, this could happen for one of three reasons:
>>> * !cpu_has_svm_nrips which can't be the case since (1) family 21
>>> supports it and (2) you actually see in the log that it does have it
>>> * next RIP is before current RIP which I think can't be the case
>>> neither because we are not looking at branch instruction or something
>>> like that.
>>> * nextrip == rip. Which I don't see how it can be true.
>>>
>>> Can you check why svm_nextrip_insn_length(v) is 0?
>>>
>> What I have found out is that vmcb->nextrip is 0 in my testing. So
>> next RIP is "before current RIP" by a very large amount.
>
>
> Ah, I just realized --- you are in VMEXIT because of #GP intercept, 
> not because of IOIO intercept. And during #GP NRIP does not get 
> updated (it is set to zero).
>
> Which means that you will always be doing decoding when you call 
> __get_instruction_length_from_list(), regardless of NRIP support.
>
>
>>> But regardless of that, how do you expect your code to work on CPUs
>>> that don't support NRIP? On those processors you *will* be decoding
>>> the instruction twice.
>>>
>> The code "works" because the only info passed between the 2 decodes is
>> the instruction length.  This is used to limit the amount of the 2nd 
>> read.
>>
>> And because svm_nextrip_insn_length(v) is 0, all the testing I have done
>> on the AMD server has been doing the decode of the instruction twice.
>>
>> If another CPU is changing the instructions as I read them (which is the
>> security issue as I understand it), all I see happing is that "wrong"
>> direction
>> or size of request could happen, or it is a vmport request or not. 
>> All of
>> which either report a #GP or do the vmport action.  Since you can do the
>> vmport action without changing the instruction bytes, I do not see how
>> the double decode opens any security issue.
>>
>> I am not trying to say this is good.  And as I replied to Jan, I am
>> looking into
>> a way to only do the single decode.  The simplest of these is to just 
>> not
>> use __get_instruction_length_from_list() and just state that on AMD the
>> instruction length is 2.  This is safe because I am only using this to
>> decide
>> much many bytes on the page to read.  The 2nd page read read depends
>> on finding a 0x66 prefix byte.
>
> Can you make this statement (about instruction length being 2) for 
> both AMD and Intel and then possibly move some code into common HVM code?
>

I could make this statement.  However there is no code movement because
of this.  And Intel uses update_guest_eip() vs AMD's
__update_guest_eip(regs, inst_len).  Since update_guest_eip() uses
the same field (VM_EXIT_INSTRUCTION_LEN).  I did not use
get_instruction_length() because I am not 100% sure that it cannot return
a 0, and get_instruction_length() would BUG in that case.  However when
update_guest_eip() is called, I know that get_instruction_length() will not
BUG.

My reading of the Intel Software Developer's Manual says that the byte
sequence "0x66 0x66 0xed" or "0x66 0x66 0x66 0xed" are valid, just
a waste of code space.  I see no reason to allow this in my decode for
VMware IN/OUT.  But the get_instruction_length() on Intel would
return 3 or 4 for these.  I plan on adjusting my unit test code to see what
VMware does in this case.

 From Intel Software Developer's Manual 2A section 2.1 I should use 5 not 2
if I am supporting this useless encoding.


> Since we are intercepting #GP on both architectures only for VMware 
> case (right?) it seems that you can just say -- "let's look at the 
> next 2 bytes and confirm that they are IN/OUT (with appropriate 
> arguments)".
>

Almost.  The #GP that I accept is only for VMware case.  When enabled you
do get all #GPs, not just the VMware ones.  And your statement is a good
description of vmport_gp_check.


    -Don Slutz

> -boris
>
>> I am just noticing that this also has the
>> side
>> effect of not injecting a #PF if the instruction is no longer readable
>> (a side
>> effect of using fetch() which uses hvm_fetch_from_guest_virt()).
>