> -----Original Message----- > From: Julien Grall <[email protected]> > Sent: 14 September 2020 11:47 > To: Paul Durrant <[email protected]>; [email protected] > Cc: Paul Durrant <[email protected]>; Jan Beulich <[email protected]>; > Andrew Cooper > <[email protected]>; Wei Liu <[email protected]>; Roger Pau Monné > <[email protected]>; George > Dunlap <[email protected]>; Ian Jackson <[email protected]>; > Stefano Stabellini > <[email protected]>; Jun Nakajima <[email protected]>; Kevin Tian > <[email protected]> > Subject: Re: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap > > Hi Paul, > > I am sorry for jumping very late in the discussion. > > On 11/09/2020 09:20, Paul Durrant wrote: > > From: Paul Durrant <[email protected]> > > > > The 'legacy' functions do implicit flushing so amend the callers to do the > > appropriate flushing. > > > > Unfortunately, because of the structure of the P2M code, we cannot remove > > the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it > > facilitates. It is now checked directly iommu_iotlb_flush(). This is safe > > because callers of iommu_iotlb_flush() on another CPU will not be affected, > > and hence a flush will be done. Also, 'iommu_dont_flush_iotlb' is now > > declared > > as bool (rather than bool_t) and setting/clearing it are no longer > > pointlessly > > gated on is_iommu_enabled() returning true. (Arguably it is also pointless > > to > > gate the call to iommu_iotlb_flush() on that condition - since it is a no-op > > in that case - but the if clause allows the scope of a stack variable to be > > restricted). > > Unfortunately, this change will re-open a potential security hole closed > by commit 671878779741: > > xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs > > When freeing a p2m entry, all the sub-tree behind it will also be > freed. > This may include intermediate page-tables or any l3 entry requiring to > drop a reference (e.g for foreign pages). As soon as pages are freed, > they may be re-used by Xen or another domain. Therefore it is necessary > to flush *all* the TLBs beforehand. > > While CPU TLBs will be flushed before freeing the pages, this is not > the case for IOMMU TLBs. This can be solved by moving the IOMMU TLBs > flush earlier in the code. > > This wasn't considered as a security issue as device passthrough on Arm > is not security supported. > > Signed-off-by: Julien Grall <[email protected]> > Tested-by: Oleksandr Tyshchenko <[email protected]> > Reviewed-by: Stefano Stabellini <[email protected]> > Release-acked-by: Juergen Gross <[email protected]> > > One possibility would be to treat iommu_dont_flush_iotlb as x86 only. >
Yes, it could be checked in the calling (and hence arch specific) code to deal with this. Paul > Cheers, > > -- > Julien Grall
