Re: [PATCH 54/60] kvm: x86: Restrict KVM planes support to KVM_IRQCHIP_SPLIT

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Redundant and mathematically unreachable `kvm->has_planes` checks in KVM ioctls.

Pre-existing issues:
- [Critical] Dangling pointer in `kvm->planes[id]` upon `anon_inode_getfile` failure leading to a Use-After-Free and Double Free.
--

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

kvm: x86: Restrict KVM planes support to KVM_IRQCHIP_SPLIT

The code right now only supports plane-aware IOAPIC IRQ routing for
IRQ-chip in split mode. Enforce that restriction in the KVM x86 code.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c6910356b0619..0b9fa10594812 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -6833,7 +6837,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		if (cap->args[0] > KVM_MAX_IRQ_ROUTES)
>  			goto split_irqchip_unlock;
>  		r = -EEXIST;
> -		if (irqchip_in_kernel(kvm))
> +		if (irqchip_in_kernel(kvm) || kvm->has_planes)

[Severity: Low]
Is this kvm->has_planes check redundant? Since kvm->has_planes is only
set to true in kvm_vm_ioctl_create_plane() which requires
kvm_arch_irqchip_in_kernel(kvm) to already be true, wouldn't
irqchip_in_kernel(kvm) always evaluate to true if has_planes is set?

>  			goto split_irqchip_unlock;
>  		if (kvm->created_vcpus)
>  			goto split_irqchip_unlock;

[ ... ]

> @@ -7398,7 +7402,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  			goto create_irqchip_unlock;
>  
>  		r = -EINVAL;
> -		if (kvm->created_vcpus)
> +		if (kvm->created_vcpus || kvm->has_planes)

[Severity: Low]
Is the kvm->has_planes check unreachable here? A few lines earlier in
this same function (inside the KVM_CREATE_IRQCHIP case), there is an
early exit:

    if (irqchip_in_kernel(kvm))
        goto create_irqchip_unlock;

Because kvm->has_planes can only be set when the irqchip is in-kernel,
wouldn't we always exit at the first check before ever reaching this
second condition?

>  			goto create_irqchip_unlock;
>  
>  		r = kvm_pic_init(kvm);

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 09fc74fe9d848..690858be8ef5b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -5497,6 +5501,7 @@ static int kvm_vm_ioctl_create_plane(struct kvm *kvm, unsigned id)
>  		goto put_kvm;
>  	}
>  
> +	kvm->has_planes = true;

[Severity: Critical]
This is a pre-existing issue, but can the error path above this line leave
a dangling pointer in kvm->planes[id]?

If anon_inode_getfile() fails, the code jumps to put_kvm and calls
kvm_destroy_one_plane(plane). However, kvm_create_plane() already
saved the pointer into the array:

virt/kvm/kvm_main.c:kvm_create_plane() {
    ...
    kvm->planes[plane_level] = plane;
    ...
}

Since kvm->planes[id] is never cleared on this error path, will a
subsequent call to kvm_destroy_planes() during VM destruction trigger
a double-free or use-after-free on this dangling pointer?

>  	fd_install(fd, file);
>  	return fd;
>

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