Re: [Xen-devel] xc_hvm_inject_trap() races

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/08/2016 11:19 AM, Razvan Cojocaru wrote:
> On 11/08/2016 10:15 AM, Jan Beulich wrote:
>>>>> On 07.11.16 at 18:01, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 11/07/2016 06:10 PM, Jan Beulich wrote:
>>>>>>> On 07.11.16 at 16:24, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> The one-shot vm_event does sound reasonable. I could set a flag
>>>>> per-VCPU, basically similar to v->arch.hvm_vcpu.inject_trap.vector, and
>>>>> fire a single event from hvm_inject_trap() if it's set (then unset it) -
>>>>> the flag would be set via an xc_monitor_next_interrupt() call in libxc.
>>>>
>>>> Doing this in hvm_inject_trap() would not cover all cases afict.
>>>> I'd suggest doing this from hvm_do_resume() _after_ the
>>>> (conditional) call to hvm_inject_trap(), if there is _any_ event
>>>> pending.
>>>
>>> But that would only cover the hypercall-injected traps. The condition in
>>> hvm_do_resume() is "if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )",
>>> and inject_trap.vector seems to only ever be set by the hypercall:
>>> [...]
>>> So if the next interrupt is not caused by the hypercall, we'll never get
>>> another event. Am I reading the code wrong?
>>
>> No, maybe I expressed myself ambiguously: I meant to say that the
>> event should be delivered from hvm_do_resume(), but _outside_ the
>> conditional guarding the call to hvm_inject_trap(). Otherwise things
>> would have been worse than when doing it inside hvm_inject_trap().
> 
> While working on this patch, I've had a new idea, which would require
> less changes and fix the problem in a more elegant manner if validated.
> Looking at vmx_idtv_reinject(), the real problem seems to be that
> VM_ENTRY_INTR_INFO is being written to directly:
> 
> 3229 static void vmx_idtv_reinject(unsigned long idtv_info)
> 3230 {
> 3231
> 3232     /* Event delivery caused this intercept? Queue for redelivery. */
> 3233     if ( unlikely(idtv_info & INTR_INFO_VALID_MASK) )
> 3234     {
> 3235         if ( hvm_event_needs_reinjection(MASK_EXTR(idtv_info,
> 3236
> INTR_INFO_INTR_TYPE_MASK),
> 3237                                          idtv_info &
> INTR_INFO_VECTOR_MASK) )
> 3238         {
> 3239             /* See SDM 3B 25.7.1.1 and .2 for info about masking
> resvd bits. */
> 3240             __vmwrite(VM_ENTRY_INTR_INFO,
> 3241                       idtv_info & ~INTR_INFO_RESVD_BITS_MASK);
> 3242             if ( idtv_info & INTR_INFO_DELIVER_CODE_MASK )
> 3243             {
> 3244                 unsigned long ec;
> 3245
> 3246                 __vmread(IDT_VECTORING_ERROR_CODE, &ec);
> 3247                 __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, ec);
> 3248             }
> 3249         }
> 3250
> 3251         /*
> 3252          * Clear NMI-blocking interruptibility info if an NMI
> delivery faulted.
> 3253          * Re-delivery will re-set it (see SDM 3B 25.7.1.2).
> 3254          */
> 3255         if ( cpu_has_vmx_vnmi &&
> 3256              ((idtv_info & INTR_INFO_INTR_TYPE_MASK) ==
> 3257               MASK_INSR(X86_EVENTTYPE_NMI, INTR_INFO_INTR_TYPE_MASK)) )
> 3258         {
> 3259             unsigned long intr_info;
> 3260
> 3261             __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_info);
> 3262             __vmwrite(GUEST_INTERRUPTIBILITY_INFO,
> 3263                       intr_info & ~VMX_INTR_SHADOW_NMI);
> 3264         }
> 3265     }
> 3266 }
> 
> where the hypercall looks at v->arch.hvm_vcpu.inject_trap.vector only.
> Then we notice that the hypercall _fails_immediately_ with -EBUSY if
> v->arch.hvm_vcpu.inject_trap.vector is already set:
> 
> 5922     case HVMOP_inject_trap:
> 5923     {
> 5924         xen_hvm_inject_trap_t tr;
> 5925         struct domain *d;
> 5926         struct vcpu *v;
> 5927
> 5928         if ( copy_from_guest(&tr, arg, 1 ) )
> 5929             return -EFAULT;
> 5930
> 5931         rc = rcu_lock_remote_domain_by_id(tr.domid, &d);
> 5932         if ( rc != 0 )
> 5933             return rc;
> 5934
> 5935         rc = -EINVAL;
> 5936         if ( !is_hvm_domain(d) )
> 5937             goto injtrap_fail;
> 5938
> 5939         rc = xsm_hvm_control(XSM_DM_PRIV, d, op);
> 5940         if ( rc )
> 5941             goto injtrap_fail;
> 5942
> 5943         rc = -ENOENT;
> 5944         if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid])
> == NULL )
> 5945             goto injtrap_fail;
> 5946
> 5947         if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
> 5948             rc = -EBUSY;

Actually the fix should be even simpler than that: we can add to this if
" || hvm_event_pending(v)".

Objections?


Thanks,
Razvan

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