Re: [Qemu-devel] [RFC 2/2] hw/i386: enforce SID verification

[Peter Xu]

On Tue, Aug 09, 2016 at 05:32:17PM +0300, David Kiarie wrote:

[...]

> @@ -2252,14 +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;
> +    VTDAddressSpace *as = opaque;
> +    uint16_t sid = pci_bus_num(as->bus) << 8 | as->devfn;

SID can be something not equals to BDF. E.g., when there are PCI
bridges. See pci_requester_id(). However...

>  
>      from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
>      from.data = (uint32_t) value;
>  
> -    if (!attrs.unspecified) {
> -        /* We have explicit Source ID */
> -        sid = attrs.requester_id;
> +    if (attrs.requester_id != sid) {
> +        VTD_DPRINTF(GENERAL, "int remap request for sid 0x%04x"
> +                    " requester_id 0x%04x couldn't be verified",
> +                    sid, attrs.requester_id);
> +        return MEMTX_ERROR;

...I am not sure whether we need extra check here. In what case will
attrs.requester_id != sid ?

Though I agree to remove the original if().

>      }
>  
>      ret = vtd_interrupt_remap_msi(opaque, &from, &to, sid);
> @@ -2325,7 +2326,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
> PCIBus *bus, int devfn)
>          memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
>                                   &s->iommu_ops, "intel_iommu", UINT64_MAX);
>          memory_region_init_io(&vtd_dev_as->iommu_ir, OBJECT(s),
> -                              &vtd_mem_ir_ops, s, "intel_iommu_ir",
> +                              &vtd_mem_ir_ops, vtd_dev_as, "intel_iommu_ir",
>                                VTD_INTERRUPT_ADDR_SIZE);
>          memory_region_add_subregion(&vtd_dev_as->iommu, 
> VTD_INTERRUPT_ADDR_FIRST,
>                                      &vtd_dev_as->iommu_ir);
> @@ -2465,6 +2466,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      vtd_init(s);
>      sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>      pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
> +    /* IOMMU expected IOAPIC SID */
> +    x86_iommu->ioapic_bdf = Q35_PSEUDO_DEVFN_IOAPIC << 8 |
> +        Q35_PSEUDO_DEVFN_IOAPIC;

We can use PCI_BUILD_BDF() here.

-- peterx