Re: [Xen-devel] [PATCH v2 1/4] x86/dom0: prevent access to MMCFG areas for PVH Dom0

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, Aug 23, 2017 at 02:16:38AM -0600, Jan Beulich wrote:
> >>> On 22.08.17 at 15:54, <roger.pau@xxxxxxxxxx> wrote:
> > On Tue, Aug 22, 2017 at 06:26:23AM -0600, Jan Beulich wrote:
> >> >>> On 11.08.17 at 18:43, <roger.pau@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/dom0_build.c
> >> > +++ b/xen/arch/x86/dom0_build.c
> >> > @@ -440,6 +440,10 @@ int __init dom0_setup_permissions(struct domain *d)
> >> >              rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
> >> >      }
> >> >  
> >> > +    /* For PVH prevent access to the MMCFG areas. */
> >> > +    if ( dom0_pvh )
> >> > +        rc |= pci_mmcfg_set_domain_permissions(d);
> >> 
> >> What about ones reported by Dom0 later on? Which then raises the
> >> question whether ...
> > 
> > This should be dealt with in the PHYSDEVOP_pci_mmcfg_reserved handler.
> > But since you propose to do white listing, I guess it doesn't matter
> > that much anymore.
> 
> Well, a fundamental question is whether white listing would work in
> the first place. I could see room for severe problems e.g. with ACPI
> methods wanting to access MMIO that's not described by any PCI
> devices' BARs. Typically that would be regions in the chipset which
> firmware is responsible for configuring/managing, the addresses of
> which can be found/set in custom config space registers.

The question would also be what would Xen allow in such white-listing.
Obviously you can get to map the same using both white-list and
black-listing (see below).

> >> > @@ -175,6 +177,25 @@ void pci_mmcfg_arch_disable(unsigned int idx)
> >> >             cfg->pci_segment, cfg->start_bus_number, 
> >> > cfg->end_bus_number);
> >> >  }
> >> >  
> >> > +int pci_mmcfg_set_domain_permissions(struct domain *d)
> >> > +{
> >> > +    unsigned int idx;
> >> > +    int rc = 0;
> >> > +
> >> > +    for ( idx = 0; idx < pci_mmcfg_config_num; idx++ )
> >> > +    {
> >> > +        const struct acpi_mcfg_allocation *cfg = 
> >> > pci_mmcfg_virt[idx].cfg;
> >> > +        unsigned long start = PFN_DOWN(cfg->address) +
> >> > +                              PCI_BDF(cfg->start_bus_number, 0, 0);
> >> > +        unsigned long end = PFN_DOWN(cfg->address) +
> >> > +                            PCI_BDF(cfg->end_bus_number, ~0, ~0);
> >> > +
> >> > +        rc |= iomem_deny_access(d, start, end);
> >> 
> >> ... this shouldn't be unnecessary by, other than PV Dom0,
> >> starting out with no I/O memory being made accessible (i.e.
> >> white listing just like we decided we would do for other
> >> properties for PVH).
> > 
> > So would you like to switch to this white listing mode even for PV
> > Dom0, or just for PVH?
> 
> No, I certainly don't think we should touch PV here.
> 
> > Should reserved regions and holes be added to it? Maybe only reserved
> > regions?
> 
> See above - reserved regions may be a minimum that's needed to
> be added, but then again we can't be certain all BIOSes properly
> report everything in use by the chipset/firmware as reserved. Otoh
> they're called reserved because no-one outside of the firmware
> should touch them.

Right. On a more general comment I can see your suspicions on this
series, TBH I don't like to implement something like this either. This
series just paper over an issue in either the VT-d IOMMU
implementation in Xen, or a hardware errata in some IOMMUs found on
older hardware.

Having that said, I've tested now a slightly less intrusive variant,
which only maps reserved regions. This will still require Xen to
blacklsit the MMCFG regions, which reside in reserved areas. Is there
anything else Xen should blacklist from reserved regions?

Roger.

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