Re: [PATCH 39/60] kvm: Make KVM_SET_GSI_ROUTING per plane

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] NULL Pointer Dereference in KVM_SET_GSI_ROUTING via uninitialized planes array
- [High] KVM_IRQ_ROUTING_IRQCHIP ignores `plane_level`, breaking intended GSI routing behavior
- [Medium] UAPI Design Flaw forces all MSI routes to a single plane

Pre-existing issues:
- [Critical] NULL Pointer Dereference in KVM_SIGNAL_MSI and KVM_CREATE_VCPU via uninitialized planes
--

commit 7d28b1526ab90d058ab359f062a984d50de2e8a8
Author: Joerg Roedel <joerg.roedel@amd.com>

Allow the KVM_SET_GSI_ROUTING on each plane.

There is still only one GSI routing table maintained per VM by the KVM
module, the plane this IOCTL was last issued at will get all GSI
interrupts.

> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -146,7 +147,7 @@ static int setup_routing_entry(struct kvm *kvm,
>  
>  	e->gsi = gsi;
>  	e->type = ue->type;
> -	r = kvm_set_routing_entry(kvm, e, ue, 0);
> +	r = kvm_set_routing_entry(kvm, e, ue, plane_level);

[Severity: High]
Does KVM_IRQ_ROUTING_IRQCHIP ignore the plane_level parameter?

While KVM_IRQ_ROUTING_MSI correctly captures plane_level, the
kvm_kernel_irq_routing_entry struct only includes plane_level for the msi
union member. The irqchip struct lacks the plane_level field:

include/linux/kvm_host.h:
    union {
        struct {
            unsigned irqchip;
            unsigned pin;
        } irqchip;

When architecture-specific handlers like kvm_set_routing_entry() process
KVM_IRQ_ROUTING_IRQCHIP, the plane_level is discarded entirely.

Does this structurally prevent binding IRQCHIP interrupts to a plane?

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4918,6 +4918,34 @@ static long __kvm_plane_ioctl(struct kvm_plane *plane, unsigned int ioctl, unsig
>  		break;
>  	}
>  #endif
> +#ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
> +	case KVM_SET_GSI_ROUTING: {
> +		void __user *argp = (void __user *)arg;
> +		struct kvm_irq_routing routing;
> +		struct kvm_irq_routing __user *urouting;
> +		struct kvm_irq_routing_entry *entries = NULL;
> +
> +		if (copy_from_user(&routing, argp, sizeof(routing)))
> +			return -EFAULT;
> +		if (!kvm_arch_can_set_irq_routing(plane->kvm) ||
> +		    routing.nr > KVM_MAX_IRQ_ROUTES ||
> +		    routing.flags)
> +			return -EINVAL;
> +		if (routing.nr) {
> +			urouting = argp;
> +			entries = vmemdup_array_user(urouting->entries,
> +						     routing.nr, sizeof(*entries));
> +			if (IS_ERR(entries)) {
> +				r = PTR_ERR(entries);
> +				return r;
> +			}
> +		}
> +		r = kvm_set_irq_routing(plane->kvm, entries, routing.nr,
> +					routing.flags, plane->level);

[Severity: Medium]
Does applying plane->level uniformly to all entries in the user payload
force all MSI routes to a single plane?

Since KVM maintains only one global routing table per VM, userspace cannot
configure a routing table where some MSIs route to plane 0 and others to
plane 1. Any subsequent call to KVM_SET_GSI_ROUTING overwrites the entire
table and binds all routes to the single plane targeted by the ioctl.

Is this intended, given it limits the ability to manage multi-plane VMs?

> +		kvfree(entries);
> +		break;
> +	}
> +#endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */
>  	default:
>  		r = -ENOTTY;
>  	}
> @@ -5506,6 +5534,9 @@ static long kvm_vm_ioctl(struct file *filp,
>  	case KVM_CREATE_VCPU:
>  #ifdef CONFIG_HAVE_KVM_MSI
>  	case KVM_SIGNAL_MSI:
> +#endif
> +#ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
> +	case KVM_SET_GSI_ROUTING:
>  #endif
>  		r = __kvm_plane_ioctl(kvm->planes[0], ioctl, arg);

[Severity: Critical]
This is a pre-existing issue, but can kvm->planes[0] be NULL here for
KVM_CREATE_VCPU and KVM_SIGNAL_MSI?

If planes are not explicitly initialized at VM creation, passing NULL to
__kvm_plane_ioctl() results in an unconditional NULL pointer dereference
when accessing plane->kvm and plane->level.

[Severity: Critical]
Does this same NULL pointer dereference now apply to KVM_SET_GSI_ROUTING?

If kvm->planes[0] defaults to NULL and is passed into __kvm_plane_ioctl(),
the new code dereferences it immediately:

__kvm_plane_ioctl()
    if (!kvm_arch_can_set_irq_routing(plane->kvm) || ...

Is there a guarantee that kvm->planes[0] is strictly initialized before
this ioctl can be called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=39