[xen staging-4.9] x86/vlapic: Bugfixes and improvements to vlapic_{read, write}()

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[xen staging-4.9] x86/vlapic: Bugfixes and improvements to vlapic_{read, write}()

patchbot
commit 1f399b907f7a8f79a72abaa69e964a47b712f5c6
Author:     Andrew Cooper <[hidden email]>
AuthorDate: Fri Sep 14 13:27:17 2018 +0200
Commit:     Jan Beulich <[hidden email]>
CommitDate: Fri Sep 14 13:27:17 2018 +0200

    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 <[hidden email]>
    Reviewed-by: Jan Beulich <[hidden email]>
    Reviewed-by: Roger Pau Monné <[hidden email]>
    master commit: b6f43c14cef3af8477a9eca4efab87dd150a2885
    master date: 2018-08-10 13:27:24 +0100
---
 xen/arch/x86/hvm/vlapic.c | 126 ++++++++++++++++++----------------------------
 1 file changed, 49 insertions(+), 77 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index aeb63d636d..350acbf21c 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -575,56 +575,37 @@ static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
     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)
@@ -806,12 +787,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,
@@ -819,49 +802,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)
@@ -975,8 +947,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)
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.9


_______________________________________________
Xen-changelog mailing list
[hidden email]
https://lists.xenproject.org/xen-changelog