On Mon, Jan 14, 2013 at 10:31 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> On Tue, Jan 08, 2013 at 06:41:51PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@xxxxxxx>
>>
>> Wire the basic framework code for VGIC support and the initial in-kernel
>> MMIO support code for the VGIC, used for the distributor emulation.
>
> [...]
>
>> +/**
>> + * vgic_reg_access - access vgic register
>> + * @mmio: pointer to the data describing the mmio access
>> + * @reg: pointer to the virtual backing of vgic distributor data
>> + * @offset: least significant 2 bits used for word offset
>> + * @mode: ACCESS_ mode (see defines above)
>> + *
>> + * Helper to make vgic register access easier using one of the access
>> + * modes defined for vgic register access
>> + * (read,raz,write-ignored,setbit,clearbit,write)
>> + */
>> +static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
>> + phys_addr_t offset, int mode)
>> +{
>> + int shift = (offset & 3) * 8;
>> + u32 mask;
>> + u32 regval;
>> +
>> + /*
>> + * Any alignment fault should have been delivered to the guest
>> + * directly (ARM ARM B3.12.7 "Prioritization of aborts").
>> + */
>> +
>> + mask = (~0U) >> shift;
>> + if (reg) {
>> + regval = *reg;
>> + } else {
>> + BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
>> + regval = 0;
>> + }
>> +
>> + if (mmio->is_write) {
>> + u32 data = (*((u32 *)mmio->data) & mask) << shift;
>> + switch (ACCESS_WRITE_MASK(mode)) {
>> + case ACCESS_WRITE_IGNORED:
>> + return;
>> +
>> + case ACCESS_WRITE_SETBIT:
>> + regval |= data;
>> + break;
>> +
>> + case ACCESS_WRITE_CLEARBIT:
>> + regval &= ~data;
>> + break;
>> +
>> + case ACCESS_WRITE_VALUE:
>> + regval = (regval & ~(mask << shift)) | data;
>> + break;
>> + }
>> + *reg = regval;
>> + } else {
>> + switch (ACCESS_READ_MASK(mode)) {
>> + case ACCESS_READ_RAZ:
>> + regval = 0;
>> + /* fall through */
>> +
>> + case ACCESS_READ_VALUE:
>> + *((u32 *)mmio->data) = (regval >> shift) & mask;
>> + }
>> + }
>> +}
>
> As I mentioned previously, I suspect that this doesn't work with big-endian
> systems. Whilst that's reasonable for the moment, a comment would be useful
> for the unlucky soul that decides to do that work in future (or add
> accessors for mmio->data as I suggested before).
>
admittedly this really hurts my brain, but I think there's actually no
problem with endianness: whatever comes in mmio->data will have native
endianness and the vgic is always little-endian, so a guest would have
to make sure to do its own endianness conversion before writing data,
or did I get this backwards? (some nasty feeling about if the OS is
compiled in another endianness than the hardware everything may
break).
Anyhow, I think there's another bug in this code though. Please take a
look and see if you agree:
commit 3cab2b93a6f6acd3c043e584f23b94ab8f1bbd66
Author: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
Date: Mon Jan 14 15:55:18 2013 -0500
KVM: ARM: Limit vgic read/writes to load/store length
The vgic read/write operations did not consider ldrb/strb masks, and
would therefore unintentionally overwrite parts of a register.
Consider for example a store of a single byte to a word-aligned address
of one of the priority registers, that would cause the 3 most
significant bytes to be overwritten with zeros.
Cc: Marc Zyniger <marc.zyngier@xxxxxxx>
Cc: Will Deacon <will.deacon@xxxxxxx>
Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 25daa07..5c1bcf5 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -233,6 +233,16 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
*vcpu, int irq)
vcpu->arch.vgic_cpu.pending_shared);
}
+static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
+{
+ return *((u32 *)mmio->data) & mask;
+}
+
+static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value)
+{
+ *((u32 *)mmio->data) = value & mask;
+}
+
/**
* vgic_reg_access - access vgic register
* @mmio: pointer to the data describing the mmio access
@@ -247,8 +257,8 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
*vcpu, int irq)
static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
phys_addr_t offset, int mode)
{
- int shift = (offset & 3) * 8;
- u32 mask;
+ int word_offset = (offset & 3) * 8;
+ u32 mask = (1UL << (mmio->len * 8)) - 1;
u32 regval;
/*
@@ -256,7 +266,6 @@ static void vgic_reg_access(struct kvm_exit_mmio
*mmio, u32 *reg,
* directly (ARM ARM B3.12.7 "Prioritization of aborts").
*/
- mask = (~0U) >> shift;
if (reg) {
regval = *reg;
} else {
@@ -265,7 +274,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
*mmio, u32 *reg,
}
if (mmio->is_write) {
- u32 data = (*((u32 *)mmio->data) & mask) << shift;
+ u32 data = mmio_data_read(mmio, mask) << word_offset;
switch (ACCESS_WRITE_MASK(mode)) {
case ACCESS_WRITE_IGNORED:
return;
@@ -279,7 +288,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
*mmio, u32 *reg,
break;
case ACCESS_WRITE_VALUE:
- regval = (regval & ~(mask << shift)) | data;
+ regval = (regval & ~(mask << word_offset)) | data;
break;
}
*reg = regval;
@@ -290,7 +299,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
*mmio, u32 *reg,
/* fall through */
case ACCESS_READ_VALUE:
- *((u32 *)mmio->data) = (regval >> shift) & mask;
+ mmio_data_write(mmio, mask, regval >> word_offset);
}
}
}
@@ -702,6 +711,12 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu,
struct kvm_run *run,
(mmio->phys_addr + mmio->len) > (base + KVM_VGIC_V2_DIST_SIZE))
return false;
+ /* We don't support ldrd / strd or ldm / stm to the emulated vgic */
+ if (mmio->len > 4) {
+ kvm_inject_dabt(vcpu, mmio->phys_addr);
+ return true;
+ }
+
range = find_matching_range(vgic_ranges, mmio, base);
if (unlikely(!range || !range->handle_mmio)) {
pr_warn("Unhandled access %d %08llx %d\n",
--
Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
