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

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

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


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