While investigating XSA-59, these were all found to be desirable adjustments. 1: x86: don''t allow Dom0 access to the MSI address range 2: x86: don''t allow Dom0 access to the HT address range 3: VT-d: warn about Compatibility Format Interrupts being enabled by firmware Signed-off-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich
2013-Aug-21 06:36 UTC
[PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
In particular, MMIO assignments should not be done using this area. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -1122,6 +1122,10 @@ int __init construct_dom0( if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) ) rc |= iomem_deny_access(dom0, mfn, mfn); } + /* MSI range. */ + rc |= iomem_deny_access(dom0, paddr_to_pfn(MSI_ADDR_BASE_LO), + paddr_to_pfn(MSI_ADDR_BASE_LO + + MSI_ADDR_DEST_ID_MASK)); /* Remove access to E820_UNUSABLE I/O regions above 1MB. */ for ( i = 0; i < e820.nr_map; i++ ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-21 06:37 UTC
[PATCH 2/3] x86: don''t allow Dom0 access to the HT address range
In particular, MMIO assignments should not be done using this area. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -1126,6 +1126,10 @@ int __init construct_dom0( rc |= iomem_deny_access(dom0, paddr_to_pfn(MSI_ADDR_BASE_LO), paddr_to_pfn(MSI_ADDR_BASE_LO + MSI_ADDR_DEST_ID_MASK)); + /* HyperTransport range. */ + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + rc |= iomem_deny_access(dom0, paddr_to_pfn(0xfdULL << 32), + paddr_to_pfn((1ULL << 40) - 1)); /* Remove access to E820_UNUSABLE I/O regions above 1MB. */ for ( i = 0; i < e820.nr_map; i++ ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-21 06:38 UTC
[PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware
... as being insecure. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -712,8 +712,8 @@ int enable_intremap(struct iommu *iommu, if ( !platform_supports_intremap() ) { - dprintk(XENLOG_ERR VTDPREFIX, - "Platform firmware does not support interrupt remapping\n"); + printk(XENLOG_ERR VTDPREFIX + " Platform firmware does not support interrupt remapping\n"); return -EINVAL; } @@ -724,15 +724,19 @@ int enable_intremap(struct iommu *iommu, if ( (sts & DMA_GSTS_IRES) && ir_ctrl->iremap_maddr ) return 0; - sts = dmar_readl(iommu->reg, DMAR_GSTS_REG); if ( !(sts & DMA_GSTS_QIES) ) { - dprintk(XENLOG_ERR VTDPREFIX, - "Queued invalidation is not enabled, should not enable " - "interrupt remapping\n"); + printk(XENLOG_ERR VTDPREFIX + " Queued invalidation is not enabled on IOMMU #%u:" + " Should not enable interrupt remapping\n", iommu->index); return -EINVAL; } + if ( !eim && (sts & DMA_GSTS_CFIS) ) + printk(XENLOG_WARNING VTDPREFIX + " Compatibility Format Interrupts permitted on IOMMU #%u:" + " Device pass-through will be insecure\n", iommu->index); + if ( ir_ctrl->iremap_maddr == 0 ) { drhd = iommu_to_drhd(iommu); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
George Dunlap
2013-Aug-21 09:13 UTC
Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware
On Wed, Aug 21, 2013 at 7:38 AM, Jan Beulich <JBeulich@suse.com> wrote:> ... as being insecure. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -712,8 +712,8 @@ int enable_intremap(struct iommu *iommu, > > if ( !platform_supports_intremap() ) > { > - dprintk(XENLOG_ERR VTDPREFIX, > - "Platform firmware does not support interrupt remapping\n"); > + printk(XENLOG_ERR VTDPREFIX > + " Platform firmware does not support interrupt remapping\n"); > return -EINVAL; > } > > @@ -724,15 +724,19 @@ int enable_intremap(struct iommu *iommu, > if ( (sts & DMA_GSTS_IRES) && ir_ctrl->iremap_maddr ) > return 0; > > - sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);Nit: Might be nice to mention the removal of the redundant read in the changelog. -George
Jan Beulich
2013-Aug-21 09:24 UTC
Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware
>>> On 21.08.13 at 11:13, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Wed, Aug 21, 2013 at 7:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >> ... as being insecure. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/drivers/passthrough/vtd/intremap.c >> +++ b/xen/drivers/passthrough/vtd/intremap.c >> @@ -712,8 +712,8 @@ int enable_intremap(struct iommu *iommu, >> >> if ( !platform_supports_intremap() ) >> { >> - dprintk(XENLOG_ERR VTDPREFIX, >> - "Platform firmware does not support interrupt remapping\n"); >> + printk(XENLOG_ERR VTDPREFIX >> + " Platform firmware does not support interrupt > remapping\n"); >> return -EINVAL; >> } >> >> @@ -724,15 +724,19 @@ int enable_intremap(struct iommu *iommu, >> if ( (sts & DMA_GSTS_IRES) && ir_ctrl->iremap_maddr ) >> return 0; >> >> - sts = dmar_readl(iommu->reg, DMAR_GSTS_REG); > > Nit: Might be nice to mention the removal of the redundant read in the > changelog.Indeed, and in fact I meant to, but forgot when I created the patch metadata. Now added. Jan
Zhang, Xiantao
2013-Aug-21 14:17 UTC
Re: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
Thanks! Acked-by Xiantao Zhang <xiantao.zhang@intel.com> Xiantao -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Wednesday, August 21, 2013 2:37 PM To: xen-devel Cc: Zhang, Xiantao; Keir Fraser Subject: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range In particular, MMIO assignments should not be done using this area. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -1122,6 +1122,10 @@ int __init construct_dom0( if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) ) rc |= iomem_deny_access(dom0, mfn, mfn); } + /* MSI range. */ + rc |= iomem_deny_access(dom0, paddr_to_pfn(MSI_ADDR_BASE_LO), + paddr_to_pfn(MSI_ADDR_BASE_LO + + MSI_ADDR_DEST_ID_MASK)); /* Remove access to E820_UNUSABLE I/O regions above 1MB. */ for ( i = 0; i < e820.nr_map; i++ )
Zhang, Xiantao
2013-Aug-21 14:18 UTC
Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware
Thanks! Acked-by Xiantao Zhang <xiantao.zhang@intel.com> Xiantao -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Wednesday, August 21, 2013 2:38 PM To: xen-devel Cc: Zhang, Xiantao; Keir Fraser Subject: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware ... as being insecure. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -712,8 +712,8 @@ int enable_intremap(struct iommu *iommu, if ( !platform_supports_intremap() ) { - dprintk(XENLOG_ERR VTDPREFIX, - "Platform firmware does not support interrupt remapping\n"); + printk(XENLOG_ERR VTDPREFIX + " Platform firmware does not support interrupt remapping\n"); return -EINVAL; } @@ -724,15 +724,19 @@ int enable_intremap(struct iommu *iommu, if ( (sts & DMA_GSTS_IRES) && ir_ctrl->iremap_maddr ) return 0; - sts = dmar_readl(iommu->reg, DMAR_GSTS_REG); if ( !(sts & DMA_GSTS_QIES) ) { - dprintk(XENLOG_ERR VTDPREFIX, - "Queued invalidation is not enabled, should not enable " - "interrupt remapping\n"); + printk(XENLOG_ERR VTDPREFIX + " Queued invalidation is not enabled on IOMMU #%u:" + " Should not enable interrupt remapping\n", iommu->index); return -EINVAL; } + if ( !eim && (sts & DMA_GSTS_CFIS) ) + printk(XENLOG_WARNING VTDPREFIX + " Compatibility Format Interrupts permitted on IOMMU #%u:" + " Device pass-through will be insecure\n", iommu->index); + if ( ir_ctrl->iremap_maddr == 0 ) { drhd = iommu_to_drhd(iommu);
Konrad Rzeszutek Wilk
2013-Aug-21 14:29 UTC
Re: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
On Wed, Aug 21, 2013 at 07:36:58AM +0100, Jan Beulich wrote:> In particular, MMIO assignments should not be done using this area.And just to make sure there are no regressions - have you tested this with an upstream dom0 kernel to make sure it does not blow things up? Or at least if it does blow up - are there any WARN or BUG to help in coming up with a patch?> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/domain_build.c > +++ b/xen/arch/x86/domain_build.c > @@ -1122,6 +1122,10 @@ int __init construct_dom0( > if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) ) > rc |= iomem_deny_access(dom0, mfn, mfn); > } > + /* MSI range. */ > + rc |= iomem_deny_access(dom0, paddr_to_pfn(MSI_ADDR_BASE_LO), > + paddr_to_pfn(MSI_ADDR_BASE_LO + > + MSI_ADDR_DEST_ID_MASK)); > > /* Remove access to E820_UNUSABLE I/O regions above 1MB. */ > for ( i = 0; i < e820.nr_map; i++ ) > > >> x86: don''t allow Dom0 access to the MSI address range > > In particular, MMIO assignments should not be done using this area. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/domain_build.c > +++ b/xen/arch/x86/domain_build.c > @@ -1122,6 +1122,10 @@ int __init construct_dom0( > if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) ) > rc |= iomem_deny_access(dom0, mfn, mfn); > } > + /* MSI range. */ > + rc |= iomem_deny_access(dom0, paddr_to_pfn(MSI_ADDR_BASE_LO), > + paddr_to_pfn(MSI_ADDR_BASE_LO + > + MSI_ADDR_DEST_ID_MASK)); > > /* Remove access to E820_UNUSABLE I/O regions above 1MB. */ > for ( i = 0; i < e820.nr_map; i++ )> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-21 14:44 UTC
Re: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
>>> On 21.08.13 at 16:29, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Wed, Aug 21, 2013 at 07:36:58AM +0100, Jan Beulich wrote: >> In particular, MMIO assignments should not be done using this area. > > And just to make sure there are no regressions - have you tested > this with an upstream dom0 kernel to make sure it does not blow things up?No, I didn''t. I''m only testing with our own kernels. But I can''t see how this could cause any regression - Dom0 is not supposed to map that page (from CPU perspective it''s the LAPIC) one, and if it ever assigned the range as MMIO to a device, things would have blown up.> Or at least if it does blow up - are there any WARN or BUG to help > in coming up with a patch?The E820 machine memory map that Dom0 gets to see would have that range marked as reserved (or unusable), and the MMIO rangeset printed in ''q'' debug key output would also show the page as not accessible. Jan
Konrad Rzeszutek Wilk
2013-Aug-21 14:48 UTC
Re: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
On Wed, Aug 21, 2013 at 03:44:09PM +0100, Jan Beulich wrote:> >>> On 21.08.13 at 16:29, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > On Wed, Aug 21, 2013 at 07:36:58AM +0100, Jan Beulich wrote: > >> In particular, MMIO assignments should not be done using this area. > > > > And just to make sure there are no regressions - have you tested > > this with an upstream dom0 kernel to make sure it does not blow things up? > > No, I didn''t. I''m only testing with our own kernels. But I can''t see > how this could cause any regression - Dom0 is not supposed to > map that page (from CPU perspective it''s the LAPIC) one, and if > it ever assigned the range as MMIO to a device, things would have > blown up.I see. What an odd name for an LAPIC region - MMIO.> > > Or at least if it does blow up - are there any WARN or BUG to help > > in coming up with a patch? > > The E820 machine memory map that Dom0 gets to see would > have that range marked as reserved (or unusable), and the > MMIO rangeset printed in ''q'' debug key output would also show > the page as not accessible.Thank you.> > Jan >
Jan Beulich
2013-Aug-21 14:55 UTC
Re: [PATCH 1/3] x86: don''t allow Dom0 access to the MSI address range
>>> On 21.08.13 at 16:48, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Wed, Aug 21, 2013 at 03:44:09PM +0100, Jan Beulich wrote: >> >>> On 21.08.13 at 16:29, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> > On Wed, Aug 21, 2013 at 07:36:58AM +0100, Jan Beulich wrote: >> >> In particular, MMIO assignments should not be done using this area. >> > >> > And just to make sure there are no regressions - have you tested >> > this with an upstream dom0 kernel to make sure it does not blow things up? >> >> No, I didn''t. I''m only testing with our own kernels. But I can''t see >> how this could cause any regression - Dom0 is not supposed to >> map that page (from CPU perspective it''s the LAPIC) one, and if >> it ever assigned the range as MMIO to a device, things would have >> blown up. > > I see. What an odd name for an LAPIC region - MMIO.The thing is that this address range has two meanings - to the CPU it''s the LAPIC range, to PCI devices it''s the MSI address one (which _is_ a special kind of MMIO). Jan