On 2016-07-21 19:53, Michael S. Tsirkin wrote:
> From: Peter Xu <pet...@redhat.com>
>
> This patch enables SID validation. Invalid interrupts will be dropped.
>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
> 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
