Re: [PATCH 03/10] KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: UAF and lockdep splat in kvm_gpc_check() due to dropped SRCU lock
- [High] kvm: x86/xen: 3-way AB-BA-CA deadlock between srcu, xen_lock, and slots_lock
--

commit 34df3b53ec91de034b47e7637dd9880ba9d6c924
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

This patch updates Xen attribute ioctls in KVM to handle memslot updates
by waiting them out (passing wait=true to kvm_gpc_refresh and
kvm_gpc_activate), instead of returning -EFAULT. It also drops the
srcu_read_lock sections around activation calls.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index fae893cd5762c..76708b8960265 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -36,7 +36,7 @@ static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu *vcpu, u64 param, u64 *r);
>  
>  DEFINE_STATIC_KEY_DEFERRED_FALSE(kvm_xen_enabled, HZ);
>  
> -static int kvm_xen_shared_info_init(struct kvm *kvm)
> +static int kvm_xen_shared_info_init(struct kvm *kvm, bool wait)
>  {
>  	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
>  	struct pvclock_wall_clock *wc;
> @@ -44,13 +44,12 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
>  	u32 wc_version;
>  	u64 wall_nsec;
>  	int ret = 0;
> -	int idx = srcu_read_lock(&kvm->srcu);
>  
>  	read_lock_irq(&gpc->lock);
>  	while (!kvm_gpc_check(gpc, PAGE_SIZE)) {
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code trigger a use-after-free and RCU lockdep splat?

Because the srcu_read_lock() was removed above, this loop calls
kvm_gpc_check() without SRCU protection. 

If we look at virt/kvm/pfncache.c:kvm_gpc_check():

bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
{
	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
...

This dereferences the memslots array and requires SRCU protection. Without it,
a concurrent memslot update can free the memslots structure before 
slots->generation is accessed.

[ ... ]

> @@ -792,19 +790,15 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>  			 * to make the call anyway.
>  			 */
>  			r = kvm->arch.xen.shinfo_cache.active ?
> -				kvm_xen_shared_info_init(kvm) : 0;
> +				kvm_xen_shared_info_init(kvm, true) : 0;
>  			mutex_unlock(&kvm->arch.xen.xen_lock);
>  		}
>  		break;
>  
>  	case KVM_XEN_ATTR_TYPE_SHARED_INFO:
>  	case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA: {
> -		int idx;
> -
>  		mutex_lock(&kvm->arch.xen.xen_lock);
>  
> -		idx = srcu_read_lock(&kvm->srcu);
> -
>  		if (data->type == KVM_XEN_ATTR_TYPE_SHARED_INFO) {
>  			gfn_t gfn = data->u.shared_info.gfn;
>  
> @@ -813,7 +807,7 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>  				r = 0;
>  			} else {
>  				r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache,
> -						     gfn_to_gpa(gfn), PAGE_SIZE, false);
> +						     gfn_to_gpa(gfn), PAGE_SIZE, true);

[Severity: High]
Does this code introduce a 3-way AB-BA-CA deadlock between srcu, xen_lock, and
slots_lock?

With wait=true, the thread executes KVM_XEN_HVM_SET_ATTR, acquires xen_lock,
and calls kvm_gpc_activate() which spins in a loop calling cond_resched()
while waiting for a memslot update to complete.

Meanwhile, a concurrent vCPU thread exiting after a WRMSR to the hypercall page
MSR might enter kvm_xen_write_hypercall_page() while holding the srcu lock:

arch/x86/kvm/xen.c:kvm_xen_write_hypercall_page() {
    ...
	mutex_lock(&kvm->arch.xen.xen_lock);

This vCPU thread blocks waiting for xen_lock, which is held by the ioctl thread
above.

A third thread performing the memslot update (holding slots_lock) calls
synchronize_srcu(&kvm->srcu), which blocks waiting for the vCPU thread to
release its srcu lock.

This creates an unrecoverable 3-way cycle where the ioctl thread waits for the
memslot update, the memslot update waits for the vCPU thread, and the vCPU
thread waits for the ioctl thread's xen_lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013204824.454208-1-dwmw2@infradead.org?part=3