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 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.


> > +        if ( dabt.size == 2 &&
> > +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> > +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> > +            goto write_ignore;
> 
> Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is
> actually for.

tr & 0xffffffff

is non-zero if any of the bytes in tr is non-zero.

((tr & 0xff) && (tr & (0xff << 8)) && (tr & (0xff << 16)) && (tr & (0xff << 
24)))

is non-zero if all the bytes in tr are non-zero.


> >          vgic_lock_rank(v, rank);
> >          if ( dabt.size == 2 )
> > -            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = 
> > *r;
> > +            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = 
> > tr;
> >          else
> >              byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - 
> > GICD_ITARGETSR)],
> > -                       *r, offset);
> > +                       tr, offset);
> >          vgic_unlock_rank(v, rank);
> >          return 1;
> >  
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index bf6fb1e..bd40628 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -227,6 +227,8 @@ int gic_irq_xlate(const u32 *intspec, unsigned int 
> > intsize,
> >                    unsigned int *out_hwirq, unsigned int *out_type);
> >  void gic_clear_lrs(struct vcpu *v);
> >  
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif
> >  
> 
> 

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