linux-kernel - Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230120084933.40f59c89.alex.williamson@redhat.com>
Date:   Fri, 20 Jan 2023 08:49:33 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Matthew Rosato <mjrosato@...ux.ibm.com>
Cc:     "Liu, Yi L" <yi.l.liu@...el.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "farman@...ux.ibm.com" <farman@...ux.ibm.com>,
        "pmorel@...ux.ibm.com" <pmorel@...ux.ibm.com>,
        "borntraeger@...ux.ibm.com" <borntraeger@...ux.ibm.com>,
        "frankja@...ux.ibm.com" <frankja@...ux.ibm.com>,
        "imbrenda@...ux.ibm.com" <imbrenda@...ux.ibm.com>,
        "david@...hat.com" <david@...hat.com>,
        "akrowiak@...ux.ibm.com" <akrowiak@...ux.ibm.com>,
        "jjherne@...ux.ibm.com" <jjherne@...ux.ibm.com>,
        "pasic@...ux.ibm.com" <pasic@...ux.ibm.com>,
        "zhenyuw@...ux.intel.com" <zhenyuw@...ux.intel.com>,
        "Wang, Zhi A" <zhi.a.wang@...el.com>,
        "Christopherson, , Sean" <seanjc@...gle.com>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kvm/vfio: Fix potential deadlock on vfio group_lock

On Fri, 20 Jan 2023 10:45:40 -0500
Matthew Rosato <mjrosato@...ux.ibm.com> wrote:

> On 1/20/23 10:08 AM, Liu, Yi L wrote:
> >> From: Liu, Yi L <yi.l.liu@...el.com>
> >> Sent: Friday, January 20, 2023 11:05 PM
> >>
> >> Currently it is possible that the final put of a KVM reference comes from
> >> vfio during its device close operation.  This occurs while the vfio group
> >> lock is held; however, if the vfio device is still in the kvm device list,
> >> then the following call chain could result in a deadlock:
> >>
> >> VFIO holds group->group_lock/group_rwsem  
> >>   -> kvm_put_kvm
> >>    -> kvm_destroy_vm
> >>     -> kvm_destroy_devices
> >>      -> kvm_vfio_destroy
> >>       -> kvm_vfio_file_set_kvm
> >>        -> vfio_file_set_kvm
> >>         -> try to hold group->group_lock/group_rwsem  
> >>
> >> The key function is the kvm_destroy_devices() which triggers destroy cb
> >> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> >> if this path doesn't call back to vfio, this dead lock would be fixed.
> >> Actually, there is a way for it. KVM provides another point to free the
> >> kvm-vfio device which is the point when the device file descriptor is
> >> closed. This can be achieved by providing the release cb instead of the
> >> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
> >>
> >> 	/*
> >> 	 * Destroy is responsible for freeing dev.
> >> 	 *
> >> 	 * Destroy may be called before or after destructors are called
> >> 	 * on emulated I/O regions, depending on whether a reference is
> >> 	 * held by a vcpu or other kvm component that gets destroyed
> >> 	 * after the emulated I/O.
> >> 	 */
> >> 	void (*destroy)(struct kvm_device *dev);
> >>
> >> 	/*
> >> 	 * Release is an alternative method to free the device. It is
> >> 	 * called when the device file descriptor is closed. Once
> >> 	 * release is called, the destroy method will not be called
> >> 	 * anymore as the device is removed from the device list of
> >> 	 * the VM. kvm->lock is held.
> >> 	 */
> >> 	void (*release)(struct kvm_device *dev);
> >>
> >> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> >> Reported-by: Alex Williamson <alex.williamson@...hat.com>
> >> Suggested-by: Kevin Tian <kevin.tian@...el.com>
> >> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> >> Signed-off-by: Yi Liu <yi.l.liu@...el.com>  
> > 
> > More background can be found in Mathew's work.
> > https://lore.kernel.org/kvm/20230114000351.115444-1-mjrosato@linux.ibm.com/T/#u
> >   
> 
> Thanks Yi.
> 
> Reviewed-by: Matthew Rosato <mjrosato@...ux.ibm.com>
> 
> One small nit:  There is a comment at the very end of
> kvm_vfio_release on the kfree(dev) that still references .destroy,
> this should be updated to .release

I've fixed this locally, s/destroy/release/ in that comment.  Thanks,

Alex