Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2019 13:08, Paul Durrant wrote:
> This patch introduces a new iommu_op to facilitate a per-implementation
> quarantine set up, and then further code for x86 implementations
> (amd and vtd) to set up a read/wrote scratch page to serve as the source/
> target for all DMA whilst a device is assigned to dom_io.

A single page in the system won't do, I'm afraid. If one guest's
(prior) device is retrying reads with data containing secrets of that
guest, another guest's (prior) device could end up writing this data
to e.g. storage where after a guest restart it is then available to
the wrong guest.

Also nit: s/wrote/write/ .

> The reason for doing this is that some hardware may continue to re-try
> DMA, despite FLR, in the event of an error. Having a scratch page mapped
> will allow pending DMA to drain and thus quiesce such buggy hardware.

Without a "sink" page mapped, this would result in IOMMU faults aiui.
What's the problem with having these faults surface and get handled,
eventually leading to the device getting bus-mastering disabled? Is
it that devices continue DMAing even when bus-mastering is off? If
so, is it even safe to pass through any such device? In any event
the description needs to be extended here.

> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

What about Arm? Can devices which Arm allows to assign to guests
also "babble" like this after de-assignment? If not, this should be
said in the description. If so, obviously that side would also want
fixing.

> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -560,6 +560,63 @@ int amd_iommu_reserve_domain_unity_map(struct domain 
> *domain,
>      return rt;
>  }
>  
> +int amd_iommu_quarantine_init(struct domain *d)

__init

> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    unsigned int level;
> +    struct amd_iommu_pte *table;
> +
> +    if ( hd->arch.root_table )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return 0;
> +    }
> +
> +    spin_lock(&hd->arch.mapping_lock);
> +
> +    level = hd->arch.paging_mode;

With DomIO being PV in principle, this is going to be the
fixed value PV domains get set, merely depending on RAM size at
boot time (i.e. not accounting for memory hotplug). This could
be easily too little for HVM guests, which are free to extend
their GFN (and hence DFN) space. Therefore I think you need to
set the maximum possible level here.

> +    hd->arch.root_table = alloc_amd_iommu_pgtable();
> +    if ( !hd->arch.root_table )
> +        goto out;
> +
> +    table = __map_domain_page(hd->arch.root_table);
> +    while ( level )
> +    {
> +        struct page_info *pg;
> +        unsigned int i;
> +
> +        /*
> +         * The pgtable allocator is fine for the leaf page, as well as
> +         * page table pages.
> +         */
> +        pg = alloc_amd_iommu_pgtable();
> +        if ( !pg )
> +            break;
> +
> +        for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
> +        {
> +            struct amd_iommu_pte *pde = &table[i];
> +
> +            set_iommu_pde_present(pde, mfn_x(page_to_mfn(pg)), level - 1,
> +                                  true, true);

This would also benefit from a comment indicating that it's fine
for the leaf level as well, despite the "pde" in the name (and
its sibling set_iommu_pte_present() actually existing). Of course
you could as well extend the comment a few lines up.

What you do need to do though is pre-fill the leaf page ...

> +        }
> +
> +        unmap_domain_page(table);
> +        table = __map_domain_page(pg);
> +        level--;
> +    }

... here, such that possible left over secrets can't end up
getting written to e.g. persistent storage or over a network.

> @@ -2683,9 +2671,68 @@ static void vtd_dump_p2m_table(struct domain *d)
>      vtd_dump_p2m_table_level(hd->arch.pgd_maddr, 
> agaw_to_level(hd->arch.agaw), 0, 0);
>  }
>  
> +static int intel_iommu_quarantine_init(struct domain *d)

__init again.

> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    struct dma_pte *parent;
> +    unsigned int level = agaw_to_level(hd->arch.agaw);

Other than for AMD this is not a problem here, as all domains
(currently) get the same AGAW. I wonder though whether precautions
would be possible here against the "normal" domain setting getting
adjusted without recalling the need to come back here.

> +    int rc;
> +
> +    if ( hd->arch.pgd_maddr )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return 0;
> +    }
> +
> +    spin_lock(&hd->arch.mapping_lock);
> +
> +    hd->arch.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
> +    if ( !hd->arch.pgd_maddr )
> +        goto out;
> +
> +    parent = (struct dma_pte *)map_vtd_domain_page(hd->arch.pgd_maddr);

Unnecessary cast; funnily enough you don't have one further down.

> +    while ( level )
> +    {
> +        uint64_t maddr;
> +        unsigned int offset;
> +
> +        /*
> +         * The pgtable allocator is fine for the leaf page, as well as
> +         * page table pages.
> +         */
> +        maddr = alloc_pgtable_maddr(1, hd->node);
> +        if ( !maddr )
> +            break;
> +
> +        for ( offset = 0; offset < PTE_NUM; offset++ )
> +        {
> +            struct dma_pte *pte = &parent[offset];
> +
> +            dma_set_pte_addr(*pte, maddr);
> +            dma_set_pte_readable(*pte);
> +            dma_set_pte_writable(*pte);
> +        }
> +        iommu_flush_cache_page(parent, 1);
> +
> +        unmap_vtd_domain_page(parent);
> +        parent = map_vtd_domain_page(maddr);
> +        level--;
> +    }

The leaf page wants scrubbing here as well.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel