Re: [Xen-devel] [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 19 Jun 2014, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Ian Campbell wrote:
> > On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> > > vgic_enable_irqs should enable irq delivery to the vcpu specified by
> > > GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER.
> > > Similarly vgic_disable_irqs should use the target vcpu specified by
> > > itarget to disable irqs.
> > > 
> > > itargets can be set to a mask but vgic_get_target_vcpu always returns
> > > the lower vcpu in the mask.
> > > 
> > > Correctly initialize itargets for SPIs.
> > > 
> > > Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > 
> > > ---
> > > 
> > > Changes in v5:
> > > - improve in-code comments;
> > > - use vgic_rank_irq;
> > > - use bit masks to write-ignore GICD_ITARGETSR;
> > > - introduce an version of vgic_get_target_vcpu that doesn't take the
> > > rank lock;
> > > - keep the rank lock while enabling/disabling irqs;
> > > - use find_first_bit instead of find_next_bit.
> > > 
> > > Changes in v4:
> > > - remove assert that could allow a guest to crash Xen;
> > > - add itargets validation to vgic_distr_mmio_write;
> > > - export vgic_get_target_vcpu.
> > > 
> > > Changes in v3:
> > > - add assert in get_target_vcpu;
> > > - rename get_target_vcpu to vgic_get_target_vcpu.
> > > 
> > > Changes in v2:
> > > - refactor the common code in get_target_vcpu;
> > > - unify PPI and SPI paths;
> > > - correctly initialize itargets for SPI;
> > > - use byte_read.
> > > ---
> > >  xen/arch/arm/vgic.c       |   71 
> > > +++++++++++++++++++++++++++++++++++++--------
> > >  xen/include/asm-arm/gic.h |    2 ++
> > >  2 files changed, 61 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 757707e..4d655af 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -111,7 +111,13 @@ int domain_vgic_init(struct domain *d)
> > >          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> > >      }
> > >      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> > > +    {
> > >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> > > +        /* By default deliver to CPU0 */
> > > +        memset(d->arch.vgic.shared_irqs[i].itargets,
> > > +               0x1,
> > > +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
> > 
> > 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
> > sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?
> 
> Yeah, I'll make the change.
> 
> 
> > > +    }
> > >      return 0;
> > >  }
> > >  
> > > @@ -374,6 +380,32 @@ read_as_zero:
> > >      return 1;
> > >  }
> > >  
> > > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int 
> > > irq)
> > > +{
> > > +    unsigned long target;
> > > +    struct vcpu *v_target;
> > > +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> > > +
> > > +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> > > +    /* 1-N SPI should be delivered as pending to all the vcpus in the
> > > +     * mask, but here we just return the first vcpu for simplicity and
> > > +     * because it would be too slow to do otherwise. */
> > > +    target = find_first_bit((const unsigned long *) &target, 8);
> > 
> > Is this definitely aligned ok?
> 
> I think so: target is unsigned long and find_first_bit expects an
> unsigned long. The problem is when target is a uint32_t or an unsigned
> int and we cast it to unsigned long on armv8.
> 
> 
> > Also the cast isn't necessary, the type is already unsigned long * and
> > if find_first_bit takes a const that'll happen automatically.
> 
> OK
> 
> 
> > > @@ -589,12 +625,23 @@ static int vgic_distr_mmio_write(struct vcpu *v, 
> > > mmio_info_t *info)
> > >          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
> > >          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
> > >          if ( rank == NULL) goto write_ignore;
> > > +        /* 8-bit vcpu mask for this domain */
> > > +        tr = (1 << v->domain->max_vcpus) - 1;
> > > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > > +        tr &= *r;
> > > +        /* ignore zero writes */
> > > +        if ( !tr )
> > > +            goto write_ignore;
> > 
> > Is this per the spec? Can you provide a reference. If not then I think
> > writing target==0 is the guest's problem.
> 
> The reference is 4.3.12 of the GIC architecture specification but
> unfortunately it is not clearly written how the gic should behave in
> case of 0 writes to GICD_ITARGETSR.
> 
> I wrote the patch thinking that a 0 write is invalid and therefore
> should be ignored. Now I am not sure anymore. You could effectively
> disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even
> need GICD_ICENABLER in that case?
> 
> That said, the current code cannot deal with itargets being 0, so this
> check is needed for security. I could change the code to be able to deal
> with invalid values or 0 values to itargets, then we could remove the
> check. I am not sure what is the best course of action.


I confirm that a real GICv2 (the one on Midway) ignores 0 writes. I
strongly suggest we do the same.

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