* Re: [PATCH] KVM: arm64: vgic-its: Reject ITS_REGS offsets above U32_MAX
2026-11-10 5:07 [PATCH] KVM: arm64: vgic-its: Reject ITS_REGS offsets above U32_MAX Dan Carpenter
@ 2026-11-10 10:59 ` Marc Zyngier
2026-11-10 13:38 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2026-11-10 10:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: Eric Auger, Oliver Upton, Fuad Tabba, Joey Gouly, Steffen Eiden,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mark Rutland, Carlos López, Sascha Bischoff, Jackie Liu,
Christoffer Dall, linux-arm-kernel, kvmarm, linux-kernel,
Karthik Bhargavan, Clément Blaudeau
On Tue, 10 Nov 2026 05:07:22 +0000,
Dan Carpenter <dan@cryspen.com> wrote:
>
> The "offset" variable is a u64 that the user controls through the
> KVM_SET_DEVICE_ATTR, KVM_GET_DEVICE_ATTR and KVM_HAS_DEVICE_ATTR
> ioctls. The problem is vgic_find_mmio_region() truncates the u64 to a
> u32, so it could return a valid region instead of failing. Then we
> use the original incorrect value of "offset" to calculate "addr".
>
> addr = its->vgic_its_base + offset;
>
> The impact of this bug is relatively minor. A user in the kvm group
> could potentially crash the VMM which they can already do without using
> a bug.
>
> Fixes: 8331c23c28e8 ("KVM: arm64: vgic-its: Implement vgic_its_has_attr_regs and attr_regs_access")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dan Carpenter <dan@cryspen.com>
> ---
> arch/arm64/kvm/vgic/vgic-its.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 5ce485485a11..532a82bed24c 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2018,6 +2018,9 @@ static int vgic_its_has_attr_regs(struct kvm_device *dev,
> gpa_t offset = attr->attr;
> int align;
>
> + if (offset > U32_MAX)
> + return -ENXIO;
> +
> align = (offset < GITS_TYPER) || (offset >= GITS_PIDR4) ? 0x3 : 0x7;
>
> if (offset & align)
> @@ -2045,6 +2048,9 @@ static int vgic_its_attr_regs_access(struct kvm_device *dev,
> its = dev->private;
> offset = attr->attr;
>
> + if (offset > U32_MAX)
> + return -ENXIO;
> +
> /*
> * Although the spec supports upper/lower 32-bit accesses to
> * 64-bit ITS registers, the userspace ABI requires 64-bit
I think this is going in the wrong direction. Instead of capping the
offset at an arbitrary value, the infrastructure should be able to
deal with arbitrary offsets.
Could you try the following (untested) hack?
I also don't think this is worth a Cc stable (the worse case is that
the VMM accesses an emulated register and sets something nonsensical),
which it can always do.
Thanks,
M.
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index fddb9da0403d5..8f9ebe241e9e4 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -820,13 +820,13 @@ void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
static int match_region(const void *key, const void *elt)
{
- const unsigned int offset = (unsigned long)key;
+ const u64 offset = (unsigned long)key;
const struct vgic_register_region *region = elt;
- if (offset < region->reg_offset)
+ if (offset < (u64)region->reg_offset)
return -1;
- if (offset >= region->reg_offset + region->len)
+ if (offset >= (u64)region->reg_offset + region->len)
return 1;
return 0;
@@ -834,9 +834,9 @@ static int match_region(const void *key, const void *elt)
const struct vgic_register_region *
vgic_find_mmio_region(const struct vgic_register_region *regions,
- int nr_regions, unsigned int offset)
+ int nr_regions, u64 offset)
{
- return bsearch((void *)(uintptr_t)offset, regions, nr_regions,
+ return bsearch((void *)offset, regions, nr_regions,
sizeof(regions[0]), match_region);
}
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.h b/arch/arm64/kvm/vgic/vgic-mmio.h
index 38ed730d68ac3..6c3c4de6883ef 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.h
+++ b/arch/arm64/kvm/vgic/vgic-mmio.h
@@ -228,6 +228,6 @@ u64 vgic_sanitise_field(u64 reg, u64 field_mask, int field_shift,
/* Find the proper register handler entry given a certain address offset */
const struct vgic_register_region *
vgic_find_mmio_region(const struct vgic_register_region *regions,
- int nr_regions, unsigned int offset);
+ int nr_regions, u64 offset);
#endif
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 3+ messages in thread