Re: [Xen-devel] [BUG] IOTLB is not flushed in IOMMU Unmap Process

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.11.13 at 15:33, CHENG Yueqiang <yqcheng.2008@xxxxxxxxxxxxxxxx> wrote:
> Bug Descriptions
> We find that Xen does NOT flush I/O TLB when iommu_unmap_page is invoked in 
> PV mode. Attackers may be able to use this bug to compromise Xen.
> (When a PV guest OS adds a new page into its page table, the hypervisor 
> should set the new page-table page read-only if the page is writable before, 
> and consequently flush the IOTLB to avoid DMA attacks. )
> 
> The following code is from __get_page_type:
> if ( unlikely((x & PGT_type_mask) != type) )
>     {
>         /* Special pages should not be accessible from devices. */
>         struct domain *d = page_get_owner(page);
>         if ( d && !is_hvm_domain(d) && unlikely(need_iommu(d)) )
>         {
>             if ( (x & PGT_type_mask) == PGT_writable_page )
>                 iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
>             else if ( type == PGT_writable_page )
>                 iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
>                                page_to_mfn(page),
>                                IOMMUF_readable|IOMMUF_writable);
>         }
> }
> 
> Details
> When Xen deals with a L2 page table entry update, the possible function flow 
> is as follows:
> do_mmu_update()
>         ->mod_l2_entry()
>                 ->get_page_from_l2e()
>                         ->get_page_and_type_from_pagenr
>                                 ->get_page_type()
>                                         ->__get_page_type
>                                                 ->iommu_unmap_page
>                                                         
> ->intel_iommu_unmap_page
>                                                                 
> ->dma_pte_clear_one()
>                                                                         
> ->__intel_iommu_iotlb_flush
>                                                                              
>    ->iommu_flush_iotlb_psi
>                                                                              
>            ->flush_iotlb_qi
> We have noticed that the function: flush_iotlb_qi is supposed to complete 
> the task of invalidating I/O TLB. However, it ALWAYS returns before really 
> flushing the IOTLB.
> In flush_iotlb_qi
> if ( flush_non_present_entry )
>         {
>         if ( !cap_caching_mode(iommu->cap) )
>             return 1;  <-- return from this point.
>         else
>             did = 0;
> }
> Similarly, the function: iommu_flush_write_buffer coming next to 
> iommu_flush_iotlb_psi directly returns also without write buffer flushing 
> because the if statement: !rwbf_quirk && !cap_rwbf(iommu->cap) are also true.
> Note that some internal information can be found at the end of this email, 
> in the log file.

I agree, albeit the capability related parts are irrelevant here.

> Fix Suggestions
> We observe that the argument: dma_old_pte_present in the function: 
> __intel_iommu_iotlb_flush is always assigned as 0 in the above function flow, 
> the value of flush_non_present_entry in flush_iotlb_qi is always 1, the 
> negation of dma_old_pte_present (!dma_old_pte_present).
> 
> We suggest to set dma_old_pte_present as 1 or collect the value from 
> dma_pte_present(pte).

Since dma_pte_present() was checked earlier on that code path,
just passing TRUE (1) here is The Right Thing To Do (TM). I'll send
a patch in a minute.

Jan


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