File 5b6d84ac-x86-fix-improve-vlapic-read-write.patch of Package xen.10697
# Commit b6f43c14cef3af8477a9eca4efab87dd150a2885
# Date 2018-08-10 13:27:24 +0100
# Author Andrew Cooper <andrew.cooper3@citrix.com>
# Committer Andrew Cooper <andrew.cooper3@citrix.com>
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>
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -587,59 +587,39 @@ static void vlapic_read_aligned(
}
}
-static int vlapic_read(
- struct vcpu *v, unsigned long address,
- unsigned long len, unsigned long *pval)
+static int vlapic_mmio_read(struct vcpu *v, unsigned long address,
+ unsigned long len, unsigned long *pval)
{
- unsigned int alignment;
- unsigned int tmp;
- unsigned long result = 0;
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int offset = address - vlapic_base_address(vlapic);
+ unsigned int alignment = offset & 0xf, result = 0;
- if ( offset > (APIC_TDCR + 0x3) )
- goto out;
-
- alignment = offset & 0x3;
-
- vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
- 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;
+ unsigned int reg;
- case 2:
- if ( alignment == 3 )
- goto unaligned_exit_and_crash;
- result = *(unsigned short *)((unsigned char *)&tmp + alignment);
- break;
+ 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=%#lx, "
- "should be 4 instead.\n", len);
- goto exit_and_crash;
+ HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#lx, "
+ "and the result is %#x", offset, len, result);
}
- HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "offset %#x with length %#lx, "
- "and the result is %#lx", offset, len, result);
-
- out:
*pval = result;
return X86EMUL_OKAY;
-
- unaligned_exit_and_crash:
- gdprintk(XENLOG_ERR, "Unaligned LAPIC read len=%#lx 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)
@@ -835,12 +815,14 @@ static int vlapic_reg_write(struct vcpu
return rc;
}
-static int vlapic_write(struct vcpu *v, unsigned long address,
- unsigned long len, unsigned long val)
+static int vlapic_mmio_write(struct vcpu *v, unsigned long address,
+ unsigned long 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 != 0xb0 )
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
@@ -848,53 +830,41 @@ static int vlapic_write(struct vcpu *v,
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.
*/
val = (uint32_t)val;
- if ( len != 4 )
+ if ( (alignment + len) <= 4 && offset <= APIC_TDCR )
{
- unsigned int tmp;
- unsigned char alignment;
-
- gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
-
- alignment = offset & 0x3;
- (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
-
- switch ( len )
+ if ( unlikely(len < 4) )
{
- case 1:
- val = ((tmp & ~(0xff << (8*alignment))) |
- ((val & 0xff) << (8*alignment)));
- break;
+ unsigned int reg;
- case 2:
- if ( alignment & 1 )
- goto unaligned_exit_and_crash;
- val = ((tmp & ~(0xffff << (8*alignment))) |
- ((val & 0xffff) << (8*alignment)));
- break;
+ vlapic_read_aligned(vlapic, offset, ®);
- default:
- gdprintk(XENLOG_ERR, "Local APIC write with len = %lx, "
- "should be 4 instead\n", len);
- goto exit_and_crash;
- }
- }
- else if ( (offset & 0x3) != 0 )
- goto unaligned_exit_and_crash;
+ alignment *= 8;
- offset &= ~0x3;
+ switch ( len )
+ {
+ case 1:
+ val = ((reg & ~(0xffU << alignment)) |
+ ((val & 0xff) << alignment));
+ break;
+
+ case 2:
+ val = ((reg & ~(0xffffU << alignment)) |
+ ((val & 0xffff) << alignment));
+ break;
+ }
+ }
- return vlapic_reg_write(v, offset, val);
+ vlapic_reg_write(v, offset, val);
+ }
- unaligned_exit_and_crash:
- gdprintk(XENLOG_ERR, "Unaligned LAPIC write len=%#lx at offset=%#x.\n",
- len, offset);
- exit_and_crash:
- domain_crash(v->domain);
- return rc;
+ return X86EMUL_OKAY;
}
int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
@@ -1002,8 +972,8 @@ static int vlapic_range(struct vcpu *v,
const struct hvm_mmio_handler vlapic_mmio_handler = {
.check_handler = vlapic_range,
- .read_handler = vlapic_read,
- .write_handler = vlapic_write
+ .read_handler = vlapic_mmio_read,
+ .write_handler = vlapic_mmio_write,
};
static void set_x2apic_id(struct vlapic *vlapic)
