Re: [PATCH v4 11/48] KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Timeout of IRS MMIO commands leads to hardware Use-After-Free for IST/VMTE memory.
- [Critical] Cache line false sharing on non-coherent GICv5 VMT causes silent memory corruption.
- [Critical] Unsafe use of kzalloc for hardware data structures requiring strict alignment.
- [High] Missing wait for idle before writing to IRS VMT configuration registers.
- [High] GICv5 probe fails systematically across kexec because it rejects an already valid VMT base register.
- [Medium] The newly allocated VMT is leaked if vgic_v5_irs_assign_vmt() returns -EBUSY and vgic_v5_irs_clear_vmt() fails.
--

--- Patch [11]: [PATCH v4 11/48] KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops ---
commit ef6f1e2217f9b0055cd57ec9bca6826104eed2cf
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops

This commit adds support for making level 1 VMTs valid, allowing dynamic
level 2 array allocation. It also implements IRS MMIO operations for
making VMs and ISTs valid or invalid.

> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c

[ ... ]

> @@ -730,8 +738,13 @@ static int vgic_v5_vmte_assign_ist(struct kvm *kvm, phys_addr_t ist_base,
>  	/* Finally, mark the entry as valid */
>  	cmd = spi_ist ? SPI_VIST_MAKE_VALID : LPI_VIST_MAKE_VALID;
>  	ret = irq_set_vcpu_affinity(vgic_v5_vpe_db(vcpu0), &cmd);
> +	if (ret) {
> +		WRITE_ONCE(vmte->val[section], 0ULL);
> +		vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +		return ret;
> +	}
>  
> -	return ret;
> +	return 0;
>  }

[Severity: Critical]
If the IRS MMIO command times out and returns an error here, KVM zeroes the
VMTE and the caller frees the IST memory. Is it possible that the hardware
IRS is still processing the delayed command?

If the hardware eventually completes the command, it might use the physical
address KVM just freed, potentially causing a Use-After-Free DMA by writing
to the freed memory.

[Severity: Critical]
Could there be a false-sharing issue leading to memory corruption on
non-coherent GICv5 VMT systems?

The vmtl2_entry structure is 32 bytes:

arch/arm64/kvm/vgic/vgic-v5-tables.h:
    struct vmtl2_entry {
        __le64 val[4];
    };

When modifying a VMTE, the cache line is flushed:

arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_clean_inval() {
    ...
    if (kvm_vgic_global_state.vgic_v5_irs_caps.non_coherent)
        dcache_clean_inval_poc(base, base + size);
}

Since ARM64 cache lines are at least 64 bytes, multiple VMTEs share a single
cache line. If the CPU writes back this dirty cache line, won't it overwrite
any concurrent updates the IRS made directly to adjacent VMTEs in main memory?

[Severity: Critical]
Are the alignment requirements for hardware data structures guaranteed when
using kzalloc()?

In vgic_v5_vmte_init(), vmd is allocated with kzalloc() and its physical
address is bit-shifted:

arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_vmte_init() {
    ...
    vmd = kzalloc(vmt_info->vmd_size, GFP_KERNEL);
    ...
    tmp = FIELD_PREP(GICV5_VMTEL2E_VMD_ADDR,
                     virt_to_phys(vmd) >> GICV5_VMTEL2E_VMD_ADDR_SHIFT);
    ...
}

Since kzalloc() only guarantees natural alignment, shifting the address right
by GICV5_VMTEL2E_VMD_ADDR_SHIFT could truncate the lower bits if the
allocation isn't perfectly aligned to 1 << SHIFT. Could this lead to the
hardware accessing the wrong memory location?

> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c

[ ... ]

> @@ -105,13 +131,55 @@ int vgic_v5_probe(const struct gic_kvm_info *info)

[ ... ]

> +	ret = vgic_v5_irs_assign_vmt(irs_caps.two_level_vmt_support,
> +				     ilog2(irs_caps.max_vms),
> +				     vgic_v5_get_vmt_base());
> +	if (ret) {
> +		kvm_err("Failed to assign the GICv5 VM tables to the IRS; no GICv5 support\n");
> +		if (!vgic_v5_irs_clear_vmt())
> +			vgic_v5_vmt_free();
> +		return -ENODEV;
> +	}

[Severity: Medium]
If vgic_v5_irs_assign_vmt() returns -EBUSY and vgic_v5_irs_clear_vmt() fails,
this skips calling vgic_v5_vmt_free().

Since vgic_v5_irs_assign_vmt() returns -EBUSY when the hardware VMT_BASER is
already valid (holding a pointer to the old VMT), doesn't skipping the free
here leak the newly allocated VMT?

[ ... ]

> @@ -153,18 +222,136 @@ int vgic_v5_probe(const struct gic_kvm_info *info)

[ ... ]

> +static int vgic_v5_irs_assign_vmt(bool two_level, u8 vm_id_bits,
> +				  phys_addr_t vmt_base)
> +{
> +	u64 vmt_baser;
> +	u32 vmt_cfgr;
> +
> +	guard(raw_spinlock_irqsave)(&global_irs_lock);
> +
> +	vmt_baser = irs_readq_relaxed(GICV5_IRS_VMT_BASER);
> +	if (!!FIELD_GET(GICV5_IRS_VMT_BASER_VALID, vmt_baser))
> +		return -EBUSY;

[Severity: High]
During a kexec, KVM does not clear this register on shutdown, so the
GICV5_IRS_VMT_BASER_VALID bit may already be set by the previous kernel.

Will vgic_v5_probe() systematically fail and permanently disable GICv5 on
kexec because it aborts instead of clearing the old VMT?

> +	vmt_cfgr = FIELD_PREP(GICV5_IRS_VMT_CFGR_VM_ID_BITS, vm_id_bits);
> +	if (two_level)
> +		vmt_cfgr |= FIELD_PREP(GICV5_IRS_VMT_CFGR_STRUCTURE,
> +				       GICV5_IRS_VMT_CFGR_STRUCTURE_TWO_LEVEL);
> +
> +	irs_writel_relaxed(vmt_cfgr, GICV5_IRS_VMT_CFGR);

[Severity: High]
Should this function call vgic_v5_irs_wait_for_vm_op() to poll for the IRS
to be idle before writing directly to GICV5_IRS_VMT_CFGR?

Other IRS writer functions enforce an idle check before writing. If the IRS
is busy processing an operation, could this direct write corrupt the hardware
state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724104819.1296803-1-sascha.bischoff@arm.com?part=11