Re: [Xen-devel] [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Zhang, Yang Z wrote on 2013-08-13:
> Jan Beulich wrote on 2013-08-12:
>>>>> On 11.08.13 at 04:59, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
>>> Jan Beulich wrote on 2013-08-09:
>>>>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
>>>>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>>>>> 
>>>>> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
>>>>> But when L2 is running, external interrupt will casue L1 vmexit
>>>>> with reason external interrupt. Then L1 will pick up the
>>>>> interrupt through vmcs. So we need to update APIC-v related
>>>>> structure accordingly;
>>>> 
>>>> This doesn't sound logical to me: If L1 picks up the interrupt
>>>> from VMCS, how can the be a reason/explanation for the need to
>>>> update the APIC-v related data?
>>> If L1 has the APIC-v, the interrupt is injected and acked by
>>> hardware normally. In this case, L1 picks up the interrupt from
>>> VMCS, but it didn't update the APIC-v related filed. Then when L1
>>> issue the EOI, since SVI is wrong, the ISR will never be cleared.
>>> Also, if the PPR and RVI are wrong, the next interrupt handle logic
>>> will totally
> wrong.
>> 
>> Just saying the same with different wording doesn't really help me
>> in this case...
> Consider the following case:
> virtual vmexit happen;
> L1 running and the vmexit reason is external interrupt;
> L1 read vector info from vmcs and go to the interrupt handler;
> Interrupt handler issues EOI to ack this interrupt; Then APIC-v
> hardware will trap the EOI and do vEOI update: set VISR[SVI]=0,
> perform PPR update and deliver the next pending interrupt.
> 
> The problem is that, SVI is not set by hardware because the interrupt
> isn't delivered through APIC-v hardware. When software to ack the
> interrupt, it will never clear the vISR successfully because the wrong
> SVI. So we need to update the SVI/RVI/PPR to the right value just like
> the interrupt is delivered through APIC-v hardware to make sure the
> following vEOI update and vPPR update correctly.
> 
>> 
>>>>> +static void nvmx_update_apicv(struct vcpu *v) { +    struct
>>>>> nestedvmx *nvmx = &vcpu_2_nvmx(v); +    struct nestedvcpu *nvcpu =
>>>>> &vcpu_nestedhvm(v); +    unsigned long reason =
>>>>> __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); +    uint32_t
>>>>> intr_info = __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + +   
>>>>> if ( !cpu_has_vmx_virtual_intr_delivery ) +        return; + +    if
>>>>> ( reason == EXIT_REASON_EXTERNAL_INTERRUPT && + nvmx->intr.source ==
>>>>> hvm_intsrc_lapic && +            (intr_info & INTR_INFO_VALID_MASK)
>>>>> ) +    { +        unsigned long status; + uint32_t rvi, ppr; +
>>>>> uint32_t vector = intr_info & 0xff; +
>>>>>  struct vlapic *vlapic = vcpu_vlapic(v); + +        WARN_ON(vector <=
>>>>> 0);
>>>> 
>>>> For both this ...
>>>> 
>>>>> +
>>>>> +        vlapic_ack_pending_irq(v, vector, 1);
>>>>> +
>>>>> +        ppr = vlapic_set_ppr(vlapic);
>>>>> +        BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>>>> 
>>>> ... and this: Is it guaranteed that the guest have no influence on
>>>> the participating values? Because otherwise either or both are
>>>> security
>> issues.
>> 
>> You didn't reply to this at all.
> The intr_info is set by hypervisor only. If guest is able to write it,
> then it should be a bug in current Xen and may cause the security issue.
> 
>>>> It also looks inconsistent to me that the former is a WARN_ON()
>>>> while the latter is a BUG_ON().
>>> If the vector is wrong, it is handled in L1 not L0, so just using
>>> WARN_ON here and L1 will handle it correctly. If PPR is wrong, then
>>> there should be somewhere wrong in L0's interrupt handle logic, so
>>> using
>> BUG_ON.
>> 
>> But you realize that if the warning triggers, it's almost guaranteed
>> that the
>> BUG_ON() would also trigger. So I continue to not be convinced.
> You are right. The two will be triggered at same time. Also, I
> realized that the vector will never less than zero since I use uint32.
> So I will remove the WARN_ON.
> As you mentioned, to avoid the security issue, it's better to use the
> WARN_ON instead the BUG_ON.
> 
> Best regards,
> Yang
Hi Jan,

Any concern about this patch? If no, I will send out the second version based 
on current comments.

Best regards,
Yang



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