Re: [PATCH v9 19/28] ARM: vITS: handle MAPD command

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 11/05/17 18:53, Andre Przywara wrote:
> The MAPD command maps a device by associating a memory region for
> storing ITEs with a certain device ID. Since it features a valid bit,
> MAPD also covers the "unmap" functionality, which we also cover here.
> We store the given guest physical address in the device table, and, if
> this command comes from Dom0, tell the host ITS driver about this new
> mapping, so it can issue the corresponding host MAPD command and create
> the required tables. We take care of rolling back actions should one
> step fail.
> Upon unmapping a device we make sure we clean up all associated
> resources and release the memory again.
> We use our existing guest memory access function to find the right ITT
> entry and store the mapping there (in guest memory).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-v3-its.c        |  18 +++++
>  xen/arch/arm/gic-v3-lpi.c        |  18 +++++
>  xen/arch/arm/vgic-v3-its.c       | 145 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic_v3_its.h |   5 ++
>  4 files changed, 186 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index fd6a394..be4c3e0 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -869,6 +869,24 @@ struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>      return get_event_pending_irq(d, vdoorbell_address, vdevid, veventid, NULL);
>  }
>
> +int gicv3_remove_guest_event(struct domain *d, paddr_t vdoorbell_address,
> +                             uint32_t vdevid, uint32_t veventid)
> +{
> +    uint32_t host_lpi = INVALID_LPI;
> +
> +    if ( !get_event_pending_irq(d, vdoorbell_address, vdevid, veventid,
> +                                &host_lpi) )
> +        return -EINVAL;
> +
> +    if ( host_lpi == INVALID_LPI )
> +        return -EINVAL;
> +
> +    gicv3_lpi_update_host_entry(host_lpi, d->domain_id,
> +                                INVALID_VCPU_ID, INVALID_LPI);
> +
> +    return 0;
> +}
> +
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
>  void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 44f6315..d427539 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -207,6 +207,24 @@ out:
>      irq_exit();
>  }
>
> +void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
> +                                 unsigned int vcpu_id, uint32_t virt_lpi)
> +{
> +    union host_lpi *hlpip, hlpi;
> +
> +    ASSERT(host_lpi >= LPI_OFFSET);
> +
> +    host_lpi -= LPI_OFFSET;
> +
> +    hlpip = &lpi_data.host_lpis[host_lpi / HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE];
> +
> +    hlpi.virt_lpi = virt_lpi;
> +    hlpi.dom_id = domain_id;
> +    hlpi.vcpu_id = vcpu_id;
> +
> +    write_u64_atomic(&hlpip->data, hlpi.data);
> +}
> +
>  static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
>  {
>      uint64_t val;
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 8a200e9..731fe0c 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -175,6 +175,21 @@ static struct vcpu *get_vcpu_from_collection(struct virt_its *its,
>  #define DEV_TABLE_ENTRY(addr, bits)                     \
>          (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(4, 0)))
>
> +/* Set the address of an ITT for a given device ID. */
> +static int its_set_itt_address(struct virt_its *its, uint32_t devid,
> +                               paddr_t itt_address, uint32_t nr_bits)
> +{
> +    paddr_t addr = get_baser_phys_addr(its->baser_dev);
> +    dev_table_entry_t itt_entry = DEV_TABLE_ENTRY(itt_address, nr_bits);
> +
> +    if ( devid >= its->max_devices )
> +        return -ENOENT;
> +
> +    return vgic_access_guest_memory(its->d,
> +                                    addr + devid * sizeof(dev_table_entry_t),
> +                                    &itt_entry, sizeof(itt_entry), true);
> +}
> +
>  /*
>   * Lookup the address of the Interrupt Translation Table associated with
>   * that device ID.
> @@ -414,6 +429,133 @@ out_unlock:
>      return ret;
>  }
>
> +/* Must be called with the ITS lock held. */
> +static int its_discard_event(struct virt_its *its,
> +                             uint32_t vdevid, uint32_t vevid)
> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +    struct vcpu *vcpu;
> +    uint32_t vlpi;
> +
> +    ASSERT(spin_is_locked(&its->its_lock));
> +
> +    if ( !read_itte_locked(its, vdevid, vevid, &vcpu, &vlpi) )
> +        return -ENOENT;
> +
> +    if ( vlpi == INVALID_LPI )
> +        return -ENOENT;
> +
> +    /* Lock this VCPU's VGIC to make sure nobody is using the pending_irq. */
> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);

There is an interesting issue happening with this code. You don't check 
the content of the memory provided by the guest. So a malicious guest 
could craft the memory in order to setup mapping with known vlpi and a 
different vCPU.

This would lead to use the wrong lock here and corrupt the list.

> +
> +    /* Remove the pending_irq from the tree. */
> +    write_lock(&its->d->arch.vgic.pend_lpi_tree_lock);
> +    p = radix_tree_delete(&its->d->arch.vgic.pend_lpi_tree, vlpi);
> +    write_unlock(&its->d->arch.vgic.pend_lpi_tree_lock);
> +
> +    if ( !p )
> +    {
> +        spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> +
> +        return -ENOENT;
> +    }
> +
> +    /* Cleanup the pending_irq and disconnect it from the LPI. */
> +    list_del_init(&p->inflight);
> +    list_del_init(&p->lr_queue);
> +    vgic_init_pending_irq(p, INVALID_LPI);
> +
> +    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> +
> +    /* Remove the corresponding host LPI entry */
> +    return gicv3_remove_guest_event(its->d, its->doorbell_address,
> +                                    vdevid, vevid);
> +}
> +
> +static int its_unmap_device(struct virt_its *its, uint32_t devid)
> +{
> +    dev_table_entry_t itt;
> +    uint64_t evid;
> +    int ret;
> +
> +    spin_lock(&its->its_lock);
> +
> +    ret = its_get_itt(its, devid, &itt);
> +    if ( ret )
> +    {
> +        spin_unlock(&its->its_lock);
> +        return ret;
> +    }
> +
> +    /*
> +     * For DomUs we need to check that the number of events per device
> +     * is really limited, otherwise looping over all events can take too
> +     * long for a guest. This ASSERT can then be removed if that is
> +     * covered.
> +     */
> +    ASSERT(is_hardware_domain(its->d));
> +
> +    for ( evid = 0; evid < DEV_TABLE_ITT_SIZE(itt); evid++ )
> +        /* Don't care about errors here, clean up as much as possible. */
> +        its_discard_event(its, devid, evid);
> +
> +    spin_unlock(&its->its_lock);
> +
> +    return 0;
> +}
> +
> +static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
> +{
> +    /* size and devid get validated by the functions called below. */
> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
> +    unsigned int size = its_cmd_get_size(cmdptr) + 1;
> +    bool valid = its_cmd_get_validbit(cmdptr);
> +    paddr_t itt_addr = its_cmd_get_ittaddr(cmdptr);
> +    int ret;
> +
> +    /* Sanitize the number of events. */
> +    if ( valid && (size > its->evid_bits) )
> +        return -1;
> +
> +    if ( !valid )
> +        /* Discard all events and remove pending LPIs. */
> +        its_unmap_device(its, devid);

its_unmap_device is returning an error but you don't check it. Please 
explain why.

> +
> +    /*
> +     * There is no easy and clean way for Xen to know the ITS device ID of a
> +     * particular (PCI) device, so we have to rely on the guest telling
> +     * us about it. For *now* we are just using the device ID *Dom0* uses,
> +     * because the driver there has the actual knowledge.
> +     * Eventually this will be replaced with a dedicated hypercall to
> +     * announce pass-through of devices.
> +     */
> +    if ( is_hardware_domain(its->d) )
> +    {
> +
> +        /*
> +         * Dom0's ITSes are mapped 1:1, so both addresses are the same.
> +         * Also the device IDs are equal.
> +         */
> +        ret = gicv3_its_map_guest_device(its->d, its->doorbell_address, devid,
> +                                         its->doorbell_address, devid,
> +                                         BIT(size), valid);
> +        if ( ret && valid )
> +            return ret;
> +    }
> +
> +    spin_lock(&its->its_lock);
> +
> +    if ( valid )
> +        ret = its_set_itt_address(its, devid, itt_addr, size);
> +    else
> +        ret = its_set_itt_address(its, devid, INVALID_PADDR, 1);
> +
> +    spin_unlock(&its->its_lock);
> +
> +    return ret;
> +}
> +
>  #define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
>  #define ITS_CMD_OFFSET(reg)             ((reg) & GENMASK(19, 5))
>
> @@ -452,6 +594,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
>          case GITS_CMD_MAPC:
>              ret = its_handle_mapc(its, command);
>              break;
> +        case GITS_CMD_MAPD:
> +            ret = its_handle_mapd(its, command);
> +            break;
>          case GITS_CMD_SYNC:
>              /* We handle ITS commands synchronously, so we ignore SYNC. */
>              break;
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index d162e89..6f94e65 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -173,6 +173,11 @@ struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>                                                      paddr_t vdoorbell_address,
>                                                      uint32_t vdevid,
>                                                      uint32_t veventid);
> +int gicv3_remove_guest_event(struct domain *d, paddr_t vdoorbell_address,
> +                                     uint32_t vdevid, uint32_t veventid);
> +void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
> +                                 unsigned int vcpu_id, uint32_t virt_lpi);
> +
>  #else
>
>  static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>

Cheers,

-- 
Julien Grall

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