Re: [PATCH v5 27/49] KVM: arm64: gic-v5: Support SPI injection

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Spectre v1 gadget in `vgic_get_irq` for GICv2/GICv3 SPIs due to improper use of `array_index_nospec`.
--

--- Patch [27]: [PATCH v5 27/49] KVM: arm64: gic-v5: Support SPI injection ---
commit 33e91ef5257fc1901a945d6c3b84eddc64cc9e09
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Support SPI injection

This commit adds support for injecting GICv5 SPIs to the core VGIC code.
It updates vgic_get_irq() to allow GICv5 SPI lookups by ID and introduces
specific irq_ops for handling their pending state without AP list queueing.

> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 023792c28fc5c..d571fafb07f32 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c

[ ... ]

> @@ -86,19 +86,31 @@ static struct vgic_irq *vgic_get_lpi(struct kvm *kvm, u32 intid)
>   */
>  struct vgic_irq *vgic_get_irq(struct kvm *kvm, u32 intid)
>  {
> -	/* Non-private IRQs are not yet implemented for GICv5 */
> -	if (vgic_is_v5(kvm))
> -		return NULL;
> +	enum kvm_device_type type = kvm->arch.vgic.vgic_model;
>  
>  	/* SPIs */
> -	if (intid >= VGIC_NR_PRIVATE_IRQS &&
> -	    intid < (kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS)) {
> -		intid = array_index_nospec(intid, kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS);
> -		return &kvm->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
> +	if (__irq_is_spi(type, intid)) {
> +		switch (type) {

[ ... ]

> +		default: {
> +			u32 max_intid = kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> +
> +			if (intid < max_intid) {
> +				intid = array_index_nospec(intid, max_intid);
> +				return &kvm->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];

[Severity: High]
This is a pre-existing issue, but does this array_index_nospec() usage leave
the kernel open to speculative out-of-bounds reads?

Since array_index_nospec() only bounds the upper limit, if a guest or
userspace ioctl triggers this path speculatively with an intid less than
VGIC_NR_PRIVATE_IRQS (e.g., 0 to 31), the value is passed through unchanged.

When the code then subtracts VGIC_NR_PRIVATE_IRQS, it underflows to a
negative index, allowing speculative access to the memory immediately
preceding the spis array. Additionally, if the CPU speculatively bypasses
the bounds check with a large intid, array_index_nospec() clamps it to 0,
which again underflows when the subtraction occurs.

Should the array_index_nospec() instead be applied to the computed array
index after the subtraction?

> +			}
> +		}}
>  	}
>  
>  	/* LPIs */

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