|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH] 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@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
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 fa43d8f..ec089cc 100644
--- 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)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |