Re: [PATCH v4 04/27] KVM: Make device name configurable

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Simultaneous KVM implementations will trigger sysfs duplicate name warnings due to hardcoded kmem cache names.

Pre-existing issues:
- [High] Missing cleanup of `kvm_debugfs_dir` on module initialization error paths leads to debugfs memory leaks and Use-After-Free vulnerabilities.
--

commit f2ad6d3ed070aa40a377cc68203ba1a3e871cc2e
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: Make device name configurable

Allow KVM implementations to choose alternative device names. This is
especially useful for architectures providing multiple KVM
implementations simultaneously.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 271884089f58d..dcad33cc2f255 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -6464,11 +6462,22 @@ void kvm_unregister_perf_callbacks(void)
>  }
>  #endif
>  
> -int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
> +int kvm_init(unsigned int vcpu_size, unsigned int vcpu_align, struct module *module)
> +{
> +	return kvm_init_with_dev(vcpu_size, vcpu_align, module, "kvm", KVM_MINOR);
> +}
> +
> +int kvm_init_with_dev(unsigned int vcpu_size, unsigned int vcpu_align,
> +		      struct module *module, const char *dev_name, int minor)
>  {
>  	int r;
>  	int cpu;
>  
> +	kvm_dev.minor = minor;
> +	kvm_dev.name = kstrdup(dev_name, GFP_KERNEL);
> +	if (!kvm_dev.name)
> +		return -ENOMEM;
> +
>  	/* A kmem cache lets us meet the alignment requirements of fx_save. */
>  	if (!vcpu_align)
>  		vcpu_align = __alignof__(struct kvm_vcpu);
> @@ -6479,8 +6488,9 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  					   offsetofend(struct kvm_vcpu, stats_id)
>  					   - offsetof(struct kvm_vcpu, arch),
>  					   NULL);

[Severity: Medium]
Since the goal is to allow multiple KVM implementations simultaneously,
will the hardcoded "kvm_vcpu" string passed to kmem_cache_create_usercopy()
in kvm_init_with_dev() cause a sysfs duplicate name warning?

If a second KVM implementation is loaded, SLUB might attempt to create a
duplicate /sys/kernel/slab/kvm_vcpu sysfs directory, triggering a warning
that could crash systems with panic_on_warn enabled. Does this need to use
the configurable dev_name instead?

> +	r = -ENOMEM;
>  	if (!kvm_vcpu_cache)
> -		return -ENOMEM;
> +		goto err_dev_name;

[ ... ]

> @@ -6547,6 +6557,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  	for_each_possible_cpu(cpu)
>  		free_cpumask_var(per_cpu(cpu_kick_mask, cpu));
>  	kmem_cache_destroy(kvm_vcpu_cache);
> +err_dev_name:
> +	kfree(kvm_dev.name);
>  	return r;
>  }

[Severity: High]
This is a pre-existing issue, but is there a missing cleanup of
kvm_debugfs_dir in the error handling path of kvm_init_with_dev()?

If initialization fails during a late step like misc_register, the error
unwinding labels are executed, but debugfs_remove_recursive() is never
called for the debugfs directory created earlier by kvm_init_debug().

Since the initialization fails and the module memory is freed, could
accessing the abandoned debugfs files later cause a use-after-free when
the kernel tries to dereference invalid fops pointers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706085229.979525-1-seiden@linux.ibm.com?part=4