Re: [Xen-devel] Xen Security Advisory 154 (CVE-2016-2270) - x86: inconsistent cachability flags on guest mappings

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Wed, 2017-01-25 at 07:21 -0700, Jan Beulich wrote:
> 
> If there wasn't INVALID_MFN to be taken care of, the !mfn_valid()
> check could simply move down, and be combined with the
> direct_mmio one.

As discussed on IRC, once we fix the overflow with order > 0, I think
INVALID_MFN is OK. Not that it should ever really happen, AFAICT. 

This seems to do the right thing for my MMIO WC test. I haven't
actually combined the !mfn_valid() check with the direct_mmio one.
Under what circumstances does that make sense anyway? For now in the
patch below I've left it *forcing* UC, unlike the direct_mmio path
which lets the guest use WC. But really, shouldn't the '!direct_mmio &&
!mfn_valid()' case just return an error?

Note that as well as using a mask for 'order' I've attempted to
consolidate the unlikely rangeset_overlaps_range() and
rangeset_contains_range() calls, on the assumption that the former will
*always* be true if the latter is, so we only need one of them in the
fast path through the function.

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 709759c..09c2f5c 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -773,20 +773,24 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
     if ( v->domain != d )
         v = d->vcpu ? d->vcpu[0] : NULL;
 
-    if ( !mfn_valid(mfn_x(mfn)) ||
-         rangeset_contains_range(mmio_ro_ranges, mfn_x(mfn),
-                                 mfn_x(mfn) + (1UL << order) - 1) )
+    /* INVALID_MFN should never happen here, but if it does then this
+     * should do the right thing instead of exploding */
+    if ( unlikely(rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
+                                         mfn_x(mfn) | ((1UL << order) - 1))) )
     {
-        *ipat = 1;
-        return MTRR_TYPE_UNCACHABLE;
+       if ( rangeset_contains_range(mmio_ro_ranges, mfn_x(mfn),
+                                    mfn_x(mfn) | ((1UL << order) - 1)) )
+       {
+           *ipat = 1;
+           return MTRR_TYPE_UNCACHABLE;
+       }
+       /* Overlaps mmio_ro_ranges but is not entirely contained. No. */
+       return -1;
     }
 
-    if ( rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
-                                 mfn_x(mfn) + (1UL << order) - 1) )
-        return -1;
-
     if ( direct_mmio )
     {
+       /* Again, INVALID_MFN should do the right thing here. */
         if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
             return MTRR_TYPE_UNCACHABLE;
         if ( order )
@@ -795,6 +799,12 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
         return MTRR_TYPE_WRBACK;
     }
 
+    if ( unlikely(!mfn_valid(mfn_x(mfn))) )
+    {
+       *ipat = 1;
+       return MTRR_TYPE_UNCACHABLE;
+    }
+
     if ( !need_iommu(d) && !cache_flush_permitted(d) )
     {
         *ipat = 1;

Attachment: smime.p7s
Description: S/MIME cryptographic signature

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