Re: [PATCH v5 20/49] KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] 64-bit IRS MMIO registers lack support for 32-bit accesses, leading to unhandled MMIO aborts and broken read-modify-write behavior.
- [Medium] Missing locking in the IRS IST MMIO read handler allows guests to observe logically torn register states.
- [Medium] Lockless, sequential updates to GICV5_IRS_CR1 can result in a torn MMIO write.
--

--- Patch [20]: [PATCH v5 20/49] KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation ---
commit e3d747ac0ec8b60b894092ad4f2dc0c7959423ef
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation
    
In order to properly support GICv5-based VMs in KVM, emulate the
CONFIG_FRAME for a virtual IRS. This emulation needs to handle guest
accesses to the MMIO region and mimic the behaviour of a real IRS.

> diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> new file mode 100644
> index 0000000000000..c6280795ab551
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c

[ ... ]

> @@ -0,0 +1,765 @@
[ ... ]
> +static void vgic_v5_mmio_write_irs_misc(struct kvm_vcpu *vcpu, gpa_t addr,
> +					unsigned int len, unsigned long val)
> +{
> +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> +	struct vgic_dist *vgic = vgic_v5_get_vgic(vcpu);
> +	const size_t offset = addr & (SZ_64K - 1);
> +
> +	switch (offset) {
[ ... ]
> +	case GICV5_IRS_CR1:
> +		irs->cr1.sh = FIELD_GET(GICV5_IRS_CR1_SH, val);
> +		irs->cr1.oc = FIELD_GET(GICV5_IRS_CR1_OC, val);
> +		irs->cr1.ic = FIELD_GET(GICV5_IRS_CR1_IC, val);
> +		irs->cr1.ist_ra = !!(val & GICV5_IRS_CR1_IST_RA);
> +		irs->cr1.ist_wa = !!(val & GICV5_IRS_CR1_IST_WA);
> +		irs->cr1.vmt_ra = !!(val & GICV5_IRS_CR1_VMT_RA);
> +		irs->cr1.vpet_ra = !!(val & GICV5_IRS_CR1_VPET_RA);
> +		irs->cr1.vmd_ra = !!(val & GICV5_IRS_CR1_VMD_RA);
> +		irs->cr1.vmd_wa = !!(val & GICV5_IRS_CR1_VMD_WA);
> +		irs->cr1.vped_ra = !!(val & GICV5_IRS_CR1_VPED_RA);
> +		irs->cr1.vped_wa = !!(val & GICV5_IRS_CR1_VPED_WA);
> +		return;

[Severity: Medium]
Since these sequential writes are performed without holding config_lock,
is there a risk of data races? If multiple vCPUs concurrently write
different values to GICV5_IRS_CR1, it seems like the internal KVM state
for irs->cr1 could end up in a torn state combining the concurrent writes.

[ ... ]

> +static unsigned long vgic_v5_mmio_read_irs_ist(struct kvm_vcpu *vcpu,
> +					       gpa_t addr, unsigned int len)
> +{
> +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> +	const size_t offset = addr & (SZ_64K - 1);
> +	u64 value = 0;
> +
> +	switch (offset) {
[ ... ]
> +	case GICV5_IRS_IST_BASER:
> +		value = FIELD_PREP(GICV5_IRS_IST_BASER_ADDR_MASK,
> +				   irs->ist_baser.addr >> GICV5_IRS_IST_BASER_ADDR_SHIFT);
> +		if (irs->ist_baser.valid)
> +			value |= GICV5_IRS_IST_BASER_VALID;
> +		break;

[Severity: Medium]
Can guests observe logically torn register states here? The write handler
vgic_v5_mmio_write_irs_ist() correctly holds config_lock to sequentially
update irs->ist_baser.valid and irs->ist_baser.addr, but this read
handler accesses them locklessly. If a vCPU reads GICV5_IRS_IST_BASER
concurrently with a write, it could see an impossible state combining the
old address with the new valid bit or vice-versa.

[ ... ]

> +static void vgic_v5_mmio_write_irs_ist(struct kvm_vcpu *vcpu, gpa_t addr,
> +				       unsigned int len, unsigned long val)
> +{
> +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> +	const size_t offset = addr & (SZ_64K - 1);
> +	enum gicv5_vcpu_cmd cmd = LPI_VIST_MAKE_INVALID;
> +
> +	switch (offset) {
[ ... ]
> +	case GICV5_IRS_IST_BASER: {

[Severity: High]
Does this strict matching on exact offsets break read-modify-write for
32-bit accesses to the high half of the register? If a guest performs a
32-bit write to offset + 4, it looks like this switch statement drops it.
Also, interpreting a 32-bit payload as a full 64-bit value without
adjusting for the offset would corrupt the register state.

[ ... ]

> +static const struct vgic_register_region vgic_v5_irs_registers[] = {
[ ... ]
> +	REGISTER_DESC_WITH_LENGTH(GICV5_IRS_IST_BASER, vgic_v5_mmio_read_irs_ist,
> +				  vgic_v5_mmio_write_irs_ist, 8,
> +				  VGIC_ACCESS_64bit),

[Severity: High]
Could omitting VGIC_ACCESS_32bit for this 64-bit register cause the KVM
IO bus to reject valid 32-bit guest accesses? If KVM's mmio dispatcher
rejects a 32-bit access, the request will fall back to userspace IO. Since
userspace does not emulate the in-kernel IRS, this usually results in an
unhandled data abort injected into the guest.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20