Re: [Xen-devel] [RFC PATCH 22/49] ARM: new VGIC: Implement virtual IRQ injection

[Julien Grall <julien.grall@xxxxxxx>]

On 27/02/18 10:17, Andre Przywara wrote:
> Hi,
Hi Andre,

> On 12/02/18 18:59, Julien Grall wrote:
> > On 09/02/18 14:39, Andre Przywara wrote:
> > >    /*
> > >     * Iterate over the VM's list of mapped LPIs to find the one with a
> > >     * matching interrupt ID and return a reference to the IRQ structure.
> > > @@ -97,6 +123,204 @@ void vgic_put_irq(struct domain *d, struct
> > > vgic_irq *irq)
> > >        xfree(irq);
> > >    }
> > >    +/**
> > > + * vgic_target_oracle - compute the target vcpu for an irq
> > > + *
> > > + * @irq:    The irq to route. Must be already locked.
> > > + *
> > > + * Based on the current state of the interrupt (enabled, pending,
> > > + * active, vcpu and target_vcpu), compute the next vcpu this should be
> > > + * given to. Return NULL if this shouldn't be injected at all.
> > > + *
> > > + * Requires the IRQ lock to be held.
> > > + */
> > > +static struct vcpu *vgic_target_oracle(struct vgic_irq *irq)
> > > +{
> > > +    ASSERT(spin_is_locked(&irq->irq_lock));
> > > +
> > > +    /* If the interrupt is active, it must stay on the current vcpu */
> > > +    if ( irq->active )
> > > +        return irq->vcpu ? : irq->target_vcpu;
> > I am not sure to understand why you check whether irq->vcpu is NULL. If
> > the interrupt is active, then irq->vcpu should be NULL. Did I miss
>                                                  ^
>                                                 not           you mean?
Yes not NULL.

> > anything?
> Not if it has been explicitly activated via ISACTIVER. This is not
> implemented in Xen at the moment, but would be in the future. So I like
> to keep this in.
Oh, I missed that case. Thank you for the explanation :).

[...]

> The list is not sorted on insertion because this is not necessary most
> of the time. In fact the hardware VGIC will do the sorting (kind of)
> within the LRs. So as long as we don't have more than <number of LRS>
> IRQs in the list, sorting is a waste of time. Experiments in the past
> showed that the number of used LRs is less than 4 almost every time. And
> since 4 is the mostly used number of implemented LRs, we virtually never
> need the sorting. So we avoid doing that on every insertion, instead
> doing that only if it's necessary.
>
> > I can foresee quite a few issues with this choice on Xen:
> >      1) You compute the size of ap list in vgic_flush_lr_state() and take
> > lock on every IRQ one by one. A guest could be nasty and make that list
> > quite big by make IRQs pending but never "active" them (i.e read IAR).
> Yeah, we could try to shortcut a bit here.
>
> >      2) This might be an issue while checking whether you need to deliver
> > an interrupt (vgic_vcpu_pending_irq) because the list is not sorted.
> Most of the time the list is very short, storing one, two or actually no
> IRQs. In the function where we check for pending IRQs we bail out as
> soon as we found the first eligible interrupt. So sorting does not help
> in the majority of cases.
As you say "most of the time". Malicious guest are unusual but just 
enough to keep busy both the hypervisor and the security team.
> If you are really concerned about that list growing too long, we could
> think about mitigations:
> 1) Try to avoid iterating the whole list while checking whether it needs
> to be sorted.
> 2) Store a flag that notes if the list has already been sorted. As long
> as we don't change anything, we don't need to sort again. Would be good
> to test whether this is actually helpful. But we would need to keep this
> flag up-to-date, which sounds a bit fragile to get right.
> 3) Switch to sort-on-insertion once we reached a certain number of IRQs
> on the list, to mitigate DOS attacks from the guest. This should avoid
> list iterations in hot paths, with IRQs disabled.
>
> But all of these sound a bit hackish to me and just would spoil the very
> clean and robust code we have today. Also I am not sure we can avoid
> list iterations every time (for instance in prune_ap_list()). There is
> an upper limit today (number of SPIs), so we might be happy with that
> for now.
> To be honest I would very much dislike changing the code at this point.
> I believe a patch series afterwards would be better, also to actually
> have some numbers on the impact of this.
More robust than the current vGIC yes. However, clean code is not an 
excuse to dismiss valid (but unusual) use case. So even if I quite like 
the new vGIC, I really don't want to deal with yet another security.
Thankfully passthrough case is not currently security supported (see 
SUPPORT.md). So we probably can defer, although I would like to keep 
track of known pitfalls of the new vGIC.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel