On Tue, Nov 01, 2016 at 06:00:08PM +0000, Andre Przywara wrote:
> In our VGIC implementation we limit the number of SPIs to a number
> that the userland application told us. Accordingly we limit the
> allocation of memory for virtual IRQs to that number.
> However in our MMIO dispatcher we didn't check if we ever access an
> IRQ beyond that limit, leading to out-of-bound accesses.
> Add a test against the number of allocated SPIs in check_region().
> Adjust the VGIC_ADDR_TO_INT macro to avoid an actual division, which
> is not implemented on ARM(32).
>
> [maz: cleaned-up original patch]
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> ---
> Hi,
>
> I checked that the old and new VGIC_ADDR_TO_INTID() algorithm give
> identical results by moving it into a small userland unit-test, using
> all <bits> values we use in the VGIC and calling it with quite some test
> addresses. No differences were found.
nice.
-Christoffer
>
> Changelog v2 .. v3:
> - further simplify VGIC_ADDR_TO_INTID
> - adjust VGIC_ADDR_TO_INTID comment
>
> Changelog v1 .. v2:
> - fix compilation for 32-bit ARM
>
> virt/kvm/arm/vgic/vgic-mmio.c | 41 +++++++++++++++++++++++++++--------------
> virt/kvm/arm/vgic/vgic-mmio.h | 14 +++++++-------
> 2 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index e18b30d..ebe1b9f 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -453,17 +453,33 @@ struct vgic_io_device *kvm_to_vgic_iodev(const struct kvm_io_device *dev)
> return container_of(dev, struct vgic_io_device, dev);
> }
>
> -static bool check_region(const struct vgic_register_region *region,
> +static bool check_region(const struct kvm *kvm,
> + const struct vgic_register_region *region,
> gpa_t addr, int len)
> {
> - if ((region->access_flags & VGIC_ACCESS_8bit) && len == 1)
> - return true;
> - if ((region->access_flags & VGIC_ACCESS_32bit) &&
> - len == sizeof(u32) && !(addr & 3))
> - return true;
> - if ((region->access_flags & VGIC_ACCESS_64bit) &&
> - len == sizeof(u64) && !(addr & 7))
> - return true;
> + int flags, nr_irqs = kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> +
> + switch (len) {
> + case sizeof(u8):
> + flags = VGIC_ACCESS_8bit;
> + break;
> + case sizeof(u32):
> + flags = VGIC_ACCESS_32bit;
> + break;
> + case sizeof(u64):
> + flags = VGIC_ACCESS_64bit;
> + break;
> + default:
> + return false;
> + }
> +
> + if ((region->access_flags & flags) && IS_ALIGNED(addr, len)) {
> + if (!region->bits_per_irq)
> + return true;
> +
> + /* Do we access a non-allocated IRQ? */
> + return VGIC_ADDR_TO_INTID(addr, region->bits_per_irq) < nr_irqs;
> + }
>
> return false;
> }
> @@ -477,7 +493,7 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>
> region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> addr - iodev->base_addr);
> - if (!region || !check_region(region, addr, len)) {
> + if (!region || !check_region(vcpu->kvm, region, addr, len)) {
> memset(val, 0, len);
> return 0;
> }
> @@ -510,10 +526,7 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>
> region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> addr - iodev->base_addr);
> - if (!region)
> - return 0;
> -
> - if (!check_region(region, addr, len))
> + if (!region || !check_region(vcpu->kvm, region, addr, len))
> return 0;
>
> switch (iodev->iodev_type) {
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 4c34d39..84961b4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -50,15 +50,15 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
> #define VGIC_ADDR_IRQ_MASK(bits) (((bits) * 1024 / 8) - 1)
>
> /*
> - * (addr & mask) gives us the byte offset for the INT ID, so we want to
> - * divide this with 'bytes per irq' to get the INT ID, which is given
> - * by '(bits) / 8'. But we do this with fixed-point-arithmetic and
> - * take advantage of the fact that division by a fraction equals
> - * multiplication with the inverted fraction, and scale up both the
> - * numerator and denominator with 8 to support at most 64 bits per IRQ:
> + * (addr & mask) gives us the _byte_ offset for the INT ID.
> + * We multiply this by 8 the get the _bit_ offset, then divide this by
> + * the number of bits to learn the actual INT ID.
> + * But instead of a division (which requires a "long long div" implementation),
> + * we shift by the binary logarithm of <bits>.
> + * This assumes that <bits> is a power of two.
> */
> #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \
> - 64 / (bits) / 8)
> + 8 >> ilog2(bits))
>
> /*
> * Some VGIC registers store per-IRQ information, with a different number
> --
> 2.9.0
>
--
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
