[PATCH 0/3] remotely XSA-59 related adjustments

[PATCH 0/3] remotely XSA-59 related adjustments

[Jan Beulich]

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>

[PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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++ )




[-- Attachment #2: x86-exclude-MSI-range.patch --]
[-- Type: text/plain, Size: 761 bytes --]

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++ )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/3] x86: don't allow Dom0 access to the HT address range

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

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++ )




[-- Attachment #2: x86-exclude-HT-range.patch --]
[-- Type: text/plain, Size: 833 bytes --]

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++ )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

... 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);




[-- Attachment #2: VT-d-check-CFIS.patch --]
[-- Type: text/plain, Size: 1582 bytes --]

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);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware

[George Dunlap]

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

Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware

[Jan Beulich]

>>> 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

Re: [PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Zhang, Xiantao]

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++ )

Re: [PATCH 3/3] VT-d: warn about Compatibility Format Interrupts being enabled by firmware

[Zhang, Xiantao]

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);

Re: [PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Konrad Rzeszutek Wilk]

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

Re: [PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Jan Beulich]

>>> 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

Re: [PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Konrad Rzeszutek Wilk]

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
> 

Re: [PATCH 1/3] x86: don't allow Dom0 access to the MSI address range

[Jan Beulich]

>>> 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