RE: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 14 September 2020 11:47
> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; 
> Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>; George
> Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; 
> Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian 
> <kevin.tian@xxxxxxxxx>
> 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 <pdurrant@xxxxxxxxxx>
> >
> > 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 <julien.grall@xxxxxxx>
>      Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>      Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>      Release-acked-by: Juergen Gross <jgross@xxxxxxxx>
> 
> 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