[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.