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 [off-list ref]
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 [off-list ref]
   Cc: Will Deacon [off-list ref]
   Signed-off-by: Christoffer Dall [off-list ref]

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
_______________________________________________
kvmarm mailing list
kvmarm at lists.cs.columbia.edu
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm