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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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?

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

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