Re: [Qemu-devel] [PULL v5 29/57] intel_iommu: add SID validation for IR

[Jan Kiszka]

On 2016-07-21 19:53, Michael S. Tsirkin wrote:
> From: Peter Xu <address@hidden>
> 
> This patch enables SID validation. Invalid interrupts will be dropped.
> 
> Signed-off-by: Peter Xu <address@hidden>
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  include/hw/i386/intel_iommu.h | 17 +++++++++++
>  hw/i386/intel_iommu.c         | 69 
> ++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 75 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 745b4e7..2eba7ed 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -102,6 +102,23 @@ struct VTDIOTLBEntry {
>      bool write_flags;
>  };
>  
> +/* VT-d Source-ID Qualifier types */
> +enum {
> +    VTD_SQ_FULL = 0x00,     /* Full SID verification */
> +    VTD_SQ_IGN_3 = 0x01,    /* Ignore bit 3 */
> +    VTD_SQ_IGN_2_3 = 0x02,  /* Ignore bits 2 & 3 */
> +    VTD_SQ_IGN_1_3 = 0x03,  /* Ignore bits 1-3 */
> +    VTD_SQ_MAX,
> +};
> +
> +/* VT-d Source Validation Types */
> +enum {
> +    VTD_SVT_NONE = 0x00,    /* No validation */
> +    VTD_SVT_ALL = 0x01,     /* Do full validation */
> +    VTD_SVT_BUS = 0x02,     /* Validate bus range */
> +    VTD_SVT_MAX,
> +};
> +
>  /* Interrupt Remapping Table Entry Definition */
>  union VTD_IRTE {
>      struct {
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a605b58..5a97548 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2008,9 +2008,13 @@ static Property vtd_properties[] = {
>  
>  /* Read IRTE entry with specific index */
>  static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
> -                        VTD_IRTE *entry)
> +                        VTD_IRTE *entry, uint16_t sid)
>  {
> +    static const uint16_t vtd_svt_mask[VTD_SQ_MAX] = \
> +        {0xffff, 0xfffb, 0xfff9, 0xfff8};
>      dma_addr_t addr = 0x00;
> +    uint16_t mask, source_id;
> +    uint8_t bus, bus_max, bus_min;
>  
>      addr = iommu->intr_root + index * sizeof(*entry);
>      if (dma_memory_read(&address_space_memory, addr, entry,
> @@ -2037,21 +2041,56 @@ static int vtd_irte_get(IntelIOMMUState *iommu, 
> uint16_t index,
>          return -VTD_FR_IR_IRTE_RSVD;
>      }
>  
> -    /*
> -     * TODO: Check Source-ID corresponds to SVT (Source Validation
> -     * Type) bits
> -     */
> +    if (sid != X86_IOMMU_SID_INVALID) {
> +        /* Validate IRTE SID */

So here you only do the validation unless sid is not invalid?! Why?
Specifically interesting because...

> +        source_id = le32_to_cpu(entry->source_id);
> +        switch (entry->sid_vtype) {
> +        case VTD_SVT_NONE:
> +            VTD_DPRINTF(IR, "No SID validation for IRTE index %d", index);
> +            break;
> +
> +        case VTD_SVT_ALL:
> +            mask = vtd_svt_mask[entry->sid_q];
> +            if ((source_id & mask) != (sid & mask)) {
> +                VTD_DPRINTF(GENERAL, "SID validation for IRTE index "
> +                            "%d failed (reqid 0x%04x sid 0x%04x)", index,
> +                            sid, source_id);
> +                return -VTD_FR_IR_SID_ERR;
> +            }
> +            break;
> +
> +        case VTD_SVT_BUS:
> +            bus_max = source_id >> 8;
> +            bus_min = source_id & 0xff;
> +            bus = sid >> 8;
> +            if (bus > bus_max || bus < bus_min) {
> +                VTD_DPRINTF(GENERAL, "SID validation for IRTE index %d "
> +                            "failed (bus %d outside %d-%d)", index, bus,
> +                            bus_min, bus_max);
> +                return -VTD_FR_IR_SID_ERR;
> +            }
> +            break;
> +
> +        default:
> +            VTD_DPRINTF(GENERAL, "Invalid SVT bits (0x%x) in IRTE index "
> +                        "%d", entry->sid_vtype, index);
> +            /* Take this as verification failure. */
> +            return -VTD_FR_IR_SID_ERR;
> +            break;
> +        }
> +    }
>  
>      return 0;
>  }
>  
>  /* Fetch IRQ information of specific IR index */
> -static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq 
> *irq)
> +static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index,
> +                             VTDIrq *irq, uint16_t sid)
>  {
>      VTD_IRTE irte = {};
>      int ret = 0;
>  
> -    ret = vtd_irte_get(iommu, index, &irte);
> +    ret = vtd_irte_get(iommu, index, &irte, sid);
>      if (ret) {
>          return ret;
>      }
> @@ -2103,7 +2142,8 @@ static void vtd_generate_msi_message(VTDIrq *irq, 
> MSIMessage *msg_out)
>  /* Interrupt remapping for MSI/MSI-X entry */
>  static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
>                                     MSIMessage *origin,
> -                                   MSIMessage *translated)
> +                                   MSIMessage *translated,
> +                                   uint16_t sid)
>  {
>      int ret = 0;
>      VTD_IR_MSIAddress addr;
> @@ -2146,7 +2186,7 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState 
> *iommu,
>          index += origin->data & VTD_IR_MSI_DATA_SUBHANDLE;
>      }
>  
> -    ret = vtd_remap_irq_get(iommu, index, &irq);
> +    ret = vtd_remap_irq_get(iommu, index, &irq, sid);
>      if (ret) {
>          return ret;
>      }
> @@ -2193,7 +2233,8 @@ do_not_translate:
>  static int vtd_int_remap(X86IOMMUState *iommu, MSIMessage *src,
>                           MSIMessage *dst, uint16_t sid)
>  {
> -    return vtd_interrupt_remap_msi(INTEL_IOMMU_DEVICE(iommu), src, dst);
> +    return vtd_interrupt_remap_msi(INTEL_IOMMU_DEVICE(iommu),
> +                                   src, dst, sid);
>  }
>  
>  static MemTxResult vtd_mem_ir_read(void *opaque, hwaddr addr,
> @@ -2209,11 +2250,17 @@ static MemTxResult vtd_mem_ir_write(void *opaque, 
> hwaddr addr,
>  {
>      int ret = 0;
>      MSIMessage from = {}, to = {};
> +    uint16_t sid = X86_IOMMU_SID_INVALID;
>  
>      from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
>      from.data = (uint32_t) value;
>  
> -    ret = vtd_interrupt_remap_msi(opaque, &from, &to);
> +    if (!attrs.unspecified) {
> +        /* We have explicit Source ID */
> +        sid = attrs.requester_id;
> +    }

...here you fall back to X86_IOMMU_SID_INVALID if writer to this region
has not provided some valid attrs. That is questionable, defeats
validation of the IOAPIC e.g. (and you can see lots of
X86_IOMMU_SID_INVALID in vtd_irte_get when booting a guest).

The credits also go to David who noticed that he still doesn't get a
proper ID from the IOAPIC while implementing AMD IR. Looks like we need
to enlighten the IOAPIC MSI writes...

Jan

> +
> +    ret = vtd_interrupt_remap_msi(opaque, &from, &to, sid);
>      if (ret) {
>          /* TODO: report error */
>          VTD_DPRINTF(GENERAL, "int remap fail for addr 0x%"PRIx64
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux