On 14/01/13 21:08, Christoffer Dall wrote:
> On Mon, Jan 14, 2013 at 10:31 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
>> 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>
Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>
> 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
>
--
Jazz is not dead. It just smells funny...
--
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
