|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 04/16] xen: Add is_vmware_port_enabled
On 09/18/14 18:53, Boris Ostrovsky wrote:
> On 09/17/2014 02:23 PM, Slutz, Donald Christopher wrote:
>> On 09/17/14 11:56, Boris Ostrovsky wrote:
>>> On 09/16/2014 08:08 AM, Slutz, Donald Christopher wrote:
>>>> On 09/12/14 09:08, Boris Ostrovsky wrote:
>>>>> On 09/11/2014 02:36 PM, Don Slutz wrote:
>>>>>> int __get_instruction_length_from_list(struct vcpu *v,
>>>>>> - const enum instruction_index *list, unsigned int
>>>>>> list_count)
>>>>>> + const enum instruction_index
>>>>>> *list,
>>>>>> + unsigned int list_count,
>>>>>> + bool_t err_rpt)
>>>>>> {
>>>>>> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>>>> unsigned int i, j, inst_len = 0;
>>>>>> @@ -211,10 +222,13 @@ int __get_instruction_length_from_list(struct
>>>>>> vcpu *v,
>>>>>> mismatch: ;
>>>>>> }
>>>>>> - gdprintk(XENLOG_WARNING,
>>>>>> - "%s: Mismatch between expected and actual instruction
>>>>>> bytes: "
>>>>>> - "eip = %lx\n", __func__, (unsigned long)vmcb->rip);
>>>>>> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>>>> + if ( err_rpt )
>>>>>> + {
>>>>>> + gdprintk(XENLOG_WARNING,
>>>>>> + "%s: Mismatch between expected and actual
>>>>>> instruction bytes: "
>>>>>> + "eip = %lx\n", __func__, (unsigned
>>>>>> long)vmcb->rip);
>>>>>> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>>>> + }
>>>>>> return 0;
>>>>>> done:
>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>>>> index b5188e6..9e14d2a 100644
>>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>>> @@ -59,6 +59,7 @@
>>>>>> #include <public/sched.h>
>>>>>> #include <asm/hvm/vpt.h>
>>>>>> #include <asm/hvm/trace.h>
>>>>>> +#include <asm/hvm/vmport.h>
>>>>>> #include <asm/hap.h>
>>>>>> #include <asm/apic.h>
>>>>>> #include <asm/debugger.h>
>>>>>> @@ -2065,6 +2066,38 @@ svm_vmexit_do_vmsave(struct vmcb_struct
>>>>>> *vmcb,
>>>>>> return;
>>>>>> }
>>>>>> +static void svm_vmexit_gp_intercept(struct cpu_user_regs *regs,
>>>>>> + struct vcpu *v)
>>>>>> +{
>>>>>> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>>>> + unsigned long inst_len;
>>>>>> + unsigned long inst_addr = svm_rip2pointer(v);
>>>>>> + int rc;
>>>>>> + static const enum instruction_index list[] = {
>>>>>> + INSTR_INL_DX, INSTR_INB_DX, INSTR_OUTL_DX, INSTR_OUTB_DX
>>>>>> + };
>>>>>> +
>>>>>> + inst_len = __get_instruction_length_from_list(
>>>>>> + v, list, ARRAY_SIZE(list), 0);
>>>>> I should have asked earlier but I don't think I understand why the
>>>>> last argument here is 0 (and therefore why you have this last
>>>>> argument
>>>>> at all).
>>>>>
>>>>> Because whether or not you are warning in
>>>>> __get_instruction_length_from_list() it will still return 0. And
>>>>> that,
>>>>> in turn, will cause vmport_gp_check() to return an error. And then
>>>>> you
>>>>> will print another warning in VMPORT_LOG. So there is a warning
>>>>> anyway.
>>>>>
>>>> A key part that you appear to have missed is that
>>>> __get_instruction_length_from_list() uses gdprintk(XENLOG_WARNING,...
>>>> but VMPORT_DBG_LOG is only available in debug=y builds. So the new
>>>> last argument is used to control this. Since this change includes
>>>> enabling #GP vmexits, it is now possible for ring 3 users to
>>>> generate at
>>>> large volume of these which with gdprintk() can flood the console.
>>> Would it be possible to decide where and whether to print the warning
>>> inside __get_instruction_length_from_list() as opposed to passing a
>>> new parameter? E.g. if vmware_port_enabled is set and list includes
>>> IN/OUT and possibly something else?
>>>
>> It could be. However the test is complex and not something I am willing
>> to be part of this patch. So the only thing I have come up with is
>> using
>> #ifndef NDEBUG around the gdprintk(). This leaves the
>>
>>
>> hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>
>> Which is not correct in this case. Now are far as I can tell,
>> calling this
>> twice does the wrong thing for this case. I.E. turns it into a double
>> fault.
>> For #GP I need to call it with vmcb->exitinfo1 instead of 0.
>>
>> (Note: v5 posted before I got this.)
>>
>> 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.
> 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. 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()).
>>
>> (hyper-0-21-54:~>cat /proc/cpuinfo
>> processor : 0
>> vendor_id : AuthenticAMD
>> cpu family : 21
>> model : 2
>> model name : AMD Opteron(tm) Processor 4365 EE
>> stepping : 0
>> microcode : 0x600081c
>> ...)
>>
>> Which means to me that by using __get_instruction_length_from_list()
>> I am reading the instruction bytes 2 times. Once in
>> __get_instruction_length_from_list() and once in vmport_gp_check().
>> This is not too bad since the rate of these is low. But this does
>> suggest to
>> me that the change to using __get_instruction_length_from_list() was
>> not the best. Having a routine that both checks the instruction and
>> reports on which one was found would be much better.
>>
>> So do I go with v5, or dropping the use of
>> __get_instruction_length_from_list()
>> in a v6? (The coding of the new routine will take some time.)
>
> Given what Jan said, it sounds like v5 is not going to work since
> you'd be decoding instruction twice, right?
>
My take is that v5 "works" because the 2nd decode does not use any info
from the 1st decode.
Sigh, I have to take that back. The check "fetch_len != inst_len" in:
if ( bytes[0] == 0x66 ) /* operand size prefix */
{
byte_cnt = 2;
i = 1;
if ( fetch_len != inst_len )
{
does violate Jan's statement. The simple change of inst_len to 2 would
fix it. As I replied to Jan, I am looking into how to not read and decode
more then one time.
-Don Slutz
> -boris
>
>>
>> -Don Slutz
>>
>>
>>> -boris
>>>
>>>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |