x86/vlapic: Bugfixes and improvements to vlapic_{read,write}()

Firstly, there is no 'offset' boundary check on the non-32-bit write path
before the call to vlapic_read_aligned(), which allows an attacker to read
beyond the end of vlapic->regs->data[], which is only 1024 bytes long.

However, as the backing memory is a domheap page, and misaligned accesses get
chunked down to single bytes across page boundaries, I can't spot any
XSA-worthy problems which occur from the overrun.

On real hardware, bad accesses don't instantly crash the machine.  Their
behaviour is undefined, but the domain_crash() prohibits sensible testing.
Behave more like other x86 MMIO and terminate bad accesses with appropriate
defaults.

While making these changes, clean up and simplify the the smaller-access
handling.  In particular, avoid pointer based mechansims for 1/2-byte reads so
as to avoid forcing the value to be spilled to the stack.

  add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-175 (-175)
  function                                     old     new   delta
  vlapic_read                                  211     142     -69
  vlapic_write                                 304     198    -106

Finally, there are a plethora of read/write functions in the vlapic namespace,
so rename these to vlapic_mmio_{read,write}() to make their purpose more
clear.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
master commit: b6f43c14cef3af8477a9eca4efab87dd150a2885
master date: 2018-08-10 13:27:24 +0100

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1b9f00a0e4ae64a0cd92c30a1dbb3979ff416749..0c7c1a818621e3c952e1de68da56a4b89580d93b 100644 (file)
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -616,56 +616,37 @@ static uint32_t vlapic_read_aligned(const struct vlapic *vlapic,
     return 0;
 }
 
-static int vlapic_read(
-    struct vcpu *v, unsigned long address,
-    unsigned int len, unsigned long *pval)
+static int vlapic_mmio_read(struct vcpu *v, unsigned long address,
+                            unsigned int len, unsigned long *pval)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     unsigned int offset = address - vlapic_base_address(vlapic);
-    unsigned int alignment = offset & 3, tmp, result = 0;
+    unsigned int alignment = offset & 0xf, result = 0;
 
-    if ( offset > (APIC_TDCR + 0x3) )
-        goto out;
-
-    tmp = vlapic_read_aligned(vlapic, offset & ~3);
-
-    switch ( len )
+    /*
+     * APIC registers are 32-bit values, aligned on 128-bit boundaries, and
+     * should be accessed with 32-bit wide loads.
+     *
+     * Some processors support smaller accesses, so we allow any access which
+     * fully fits within the 32-bit register.
+     */
+    if ( (alignment + len) <= 4 && offset <= (APIC_TDCR + 3) )
     {
-    case 1:
-        result = *((unsigned char *)&tmp + alignment);
-        break;
-
-    case 2:
-        if ( alignment == 3 )
-            goto unaligned_exit_and_crash;
-        result = *(unsigned short *)((unsigned char *)&tmp + alignment);
-        break;
+        uint32_t reg = vlapic_read_aligned(vlapic, offset & ~0xf);
 
-    case 4:
-        if ( alignment != 0 )
-            goto unaligned_exit_and_crash;
-        result = *(unsigned int *)((unsigned char *)&tmp + alignment);
-        break;
+        switch ( len )
+        {
+        case 1: result = (uint8_t) (reg >> (alignment * 8)); break;
+        case 2: result = (uint16_t)(reg >> (alignment * 8)); break;
+        case 4: result = reg;                                break;
+        }
 
-    default:
-        gdprintk(XENLOG_ERR, "Local APIC read with len=%#x, "
-                 "should be 4 instead.\n", len);
-        goto exit_and_crash;
+        HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#x, "
+                    "and the result is %#x", offset, len, result);
     }
 
-    HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#x, "
-                "and the result is %#x", offset, len, result);
-
- out:
     *pval = result;
     return X86EMUL_OKAY;
-
- unaligned_exit_and_crash:
-    gdprintk(XENLOG_ERR, "Unaligned LAPIC read len=%#x at offset=%#x.\n",
-             len, offset);
- exit_and_crash:
-    domain_crash(v->domain);
-    return X86EMUL_OKAY;
 }
 
 int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
@@ -908,12 +889,14 @@ static void vlapic_reg_write(struct vcpu *v,
     }
 }
 
-static int vlapic_write(struct vcpu *v, unsigned long address,
-                        unsigned int len, unsigned long val)
+static int vlapic_mmio_write(struct vcpu *v, unsigned long address,
+                             unsigned int len, unsigned long val)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     unsigned int offset = address - vlapic_base_address(vlapic);
-    int rc = X86EMUL_OKAY;
+    unsigned int alignment = offset & 0xf;
+
+    offset &= ~0xf;
 
     if ( offset != APIC_EOI )
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
@@ -921,49 +904,38 @@ static int vlapic_write(struct vcpu *v, unsigned long address,
                     offset, len, val);
 
     /*
-     * According to the IA32 Manual, all accesses should be 32 bits.
-     * Some OSes do 8- or 16-byte accesses, however.
+     * APIC registers are 32-bit values, aligned on 128-bit boundaries, and
+     * should be accessed with 32-bit wide stores.
+     *
+     * Some processors support smaller accesses, so we allow any access which
+     * fully fits within the 32-bit register.
      */
-    if ( unlikely(len != 4) )
+    if ( (alignment + len) <= 4 && offset <= APIC_TDCR )
     {
-        unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
-        unsigned char alignment = (offset & 3) * 8;
-
-        switch ( len )
+        if ( unlikely(len < 4) )
         {
-        case 1:
-            val = ((tmp & ~(0xffU << alignment)) |
-                   ((val & 0xff) << alignment));
-            break;
+            uint32_t reg = vlapic_read_aligned(vlapic, offset);
 
-        case 2:
-            if ( alignment & 1 )
-                goto unaligned_exit_and_crash;
-            val = ((tmp & ~(0xffffU << alignment)) |
-                   ((val & 0xffff) << alignment));
-            break;
+            alignment *= 8;
 
-        default:
-            gprintk(XENLOG_ERR, "LAPIC write with len %u\n", len);
-            goto exit_and_crash;
+            switch ( len )
+            {
+            case 1:
+                val = ((reg & ~(0xffU << alignment)) |
+                       ((val &  0xff) << alignment));
+                break;
+
+            case 2:
+                val = ((reg & ~(0xffffU << alignment)) |
+                       ((val &  0xffff) << alignment));
+                break;
+            }
         }
 
-        gdprintk(XENLOG_INFO, "Notice: LAPIC write with len %u\n", len);
-        offset &= ~3;
+        vlapic_reg_write(v, offset, val);
     }
-    else if ( unlikely(offset & 3) )
-        goto unaligned_exit_and_crash;
-
-    vlapic_reg_write(v, offset, val);
 
     return X86EMUL_OKAY;
-
- unaligned_exit_and_crash:
-    gprintk(XENLOG_ERR, "Unaligned LAPIC write: len=%u offset=%#x.\n",
-            len, offset);
- exit_and_crash:
-    domain_crash(v->domain);
-    return rc;
 }
 
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
@@ -1077,8 +1049,8 @@ static int vlapic_range(struct vcpu *v, unsigned long addr)
 
 static const struct hvm_mmio_ops vlapic_mmio_ops = {
     .check = vlapic_range,
-    .read = vlapic_read,
-    .write = vlapic_write
+    .read = vlapic_mmio_read,
+    .write = vlapic_mmio_write,
 };
 
 static void set_x2apic_id(struct vlapic *vlapic)