Re: [PATCH v4 05/21] IOMMU/x86: restrict IO-APIC mappings for PV Dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 04, 2022 at 11:32:51AM +0200, Jan Beulich wrote:
> On 03.05.2022 16:50, Jan Beulich wrote:
> > On 03.05.2022 15:00, Roger Pau Monné wrote:
> >> On Mon, Apr 25, 2022 at 10:34:23AM +0200, Jan Beulich wrote:
> >>> While already the case for PVH, there's no reason to treat PV
> >>> differently here, though of course the addresses get taken from another
> >>> source in this case. Except that, to match CPU side mappings, by default
> >>> we permit r/o ones. This then also means we now deal consistently with
> >>> IO-APICs whose MMIO is or is not covered by E820 reserved regions.
> >>>
> >>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>> ---
> >>> [integrated] v1: Integrate into series.
> >>> [standalone] v2: Keep IOMMU mappings in sync with CPU ones.
> >>>
> >>> --- a/xen/drivers/passthrough/x86/iommu.c
> >>> +++ b/xen/drivers/passthrough/x86/iommu.c
> >>> @@ -275,12 +275,12 @@ void iommu_identity_map_teardown(struct
> >>>      }
> >>>  }
> >>>  
> >>> -static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
> >>> -                                         unsigned long pfn,
> >>> -                                         unsigned long max_pfn)
> >>> +static unsigned int __hwdom_init hwdom_iommu_map(const struct domain *d,
> >>> +                                                 unsigned long pfn,
> >>> +                                                 unsigned long max_pfn)
> >>>  {
> >>>      mfn_t mfn = _mfn(pfn);
> >>> -    unsigned int i, type;
> >>> +    unsigned int i, type, perms = IOMMUF_readable | IOMMUF_writable;
> >>>  
> >>>      /*
> >>>       * Set up 1:1 mapping for dom0. Default to include only conventional 
> >>> RAM
> >>> @@ -289,44 +289,60 @@ static bool __hwdom_init hwdom_iommu_map
> >>>       * that fall in unusable ranges for PV Dom0.
> >>>       */
> >>>      if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) )
> >>> -        return false;
> >>> +        return 0;
> >>>  
> >>>      switch ( type = page_get_ram_type(mfn) )
> >>>      {
> >>>      case RAM_TYPE_UNUSABLE:
> >>> -        return false;
> >>> +        return 0;
> >>>  
> >>>      case RAM_TYPE_CONVENTIONAL:
> >>>          if ( iommu_hwdom_strict )
> >>> -            return false;
> >>> +            return 0;
> >>>          break;
> >>>  
> >>>      default:
> >>>          if ( type & RAM_TYPE_RESERVED )
> >>>          {
> >>>              if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> >>> -                return false;
> >>> +                perms = 0;
> >>>          }
> >>> -        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > 
> >>> max_pfn )
> >>> -            return false;
> >>> +        else if ( is_hvm_domain(d) )
> >>> +            return 0;
> >>> +        else if ( !iommu_hwdom_inclusive || pfn > max_pfn )
> >>> +            perms = 0;
> >>>      }
> >>>  
> >>>      /* Check that it doesn't overlap with the Interrupt Address Range. */
> >>>      if ( pfn >= 0xfee00 && pfn <= 0xfeeff )
> >>> -        return false;
> >>> +        return 0;
> >>>      /* ... or the IO-APIC */
> >>> -    for ( i = 0; has_vioapic(d) && i < d->arch.hvm.nr_vioapics; i++ )
> >>> -        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>> -            return false;
> >>> +    if ( has_vioapic(d) )
> >>> +    {
> >>> +        for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
> >>> +            if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>> +                return 0;
> >>> +    }
> >>> +    else if ( is_pv_domain(d) )
> >>> +    {
> >>> +        /*
> >>> +         * Be consistent with CPU mappings: Dom0 is permitted to 
> >>> establish r/o
> >>> +         * ones there, so it should also have such established for 
> >>> IOMMUs.
> >>> +         */
> >>> +        for ( i = 0; i < nr_ioapics; i++ )
> >>> +            if ( pfn == PFN_DOWN(mp_ioapics[i].mpc_apicaddr) )
> >>> +                return rangeset_contains_singleton(mmio_ro_ranges, pfn)
> >>> +                       ? IOMMUF_readable : 0;
> >>
> >> If we really are after consistency with CPU side mappings, we should
> >> likely take the whole contents of mmio_ro_ranges and d->iomem_caps
> >> into account, not just the pages belonging to the IO-APIC?
> >>
> >> There could also be HPET pages mapped as RO for PV.
> > 
> > Hmm. This would be a yet bigger functional change, but indeed would further
> > improve consistency. But shouldn't we then also establish r/w mappings for
> > stuff in ->iomem_caps but not in mmio_ro_ranges? This would feel like going
> > too far ...
> 
> FTAOD I didn't mean to say that I think such mappings shouldn't be there;
> I have been of the opinion that e.g. I/O directly to/from the linear
> frame buffer of a graphics device should in principle be permitted. But
> which specific mappings to put in place can imo not be derived from
> ->iomem_caps, as we merely subtract certain ranges after initially having
> set all bits in it. Besides ranges not mapping any MMIO, even something
> like the PCI ECAM ranges (parts of which we may also force to r/o, and
> which we would hence cover here if I followed your suggestion) are
> questionable in this regard.

Right, ->iomem_caps is indeed too wide for our purpose.  What
about using something like:

else if ( is_pv_domain(d) )
{
    if ( !iomem_access_permitted(d, pfn, pfn) )
        return 0;
    if ( rangeset_contains_singleton(mmio_ro_ranges, pfn) )
        return IOMMUF_readable;
}

That would get us a bit closer to allowed CPU side mappings, and we
don't need to special case IO-APIC or HPET addresses as those are
already added to ->iomem_caps or mmio_ro_ranges respectively by
dom0_setup_permissions().

Thanks, Roger.