| 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
| ||
|
Date: Mon, 1 Jul 2024 18:30:05 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: "Tian, Kevin" <kevin.tian@...el.com>, "Zhao, Yan Y"
<yan.y.zhao@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"jgg@...dia.com" <jgg@...dia.com>
Subject: Re: [PATCH] vfio: Get/put KVM only for the first/last
vfio_df_open/close in cdev path
On 2024/7/1 16:43, Tian, Kevin wrote:
>> From: Zhao, Yan Y <yan.y.zhao@...el.com>
>> Sent: Friday, June 28, 2024 11:19 PM
>>
>> In the device cdev path, adjust the handling of the KVM reference count to
>> only increment with the first vfio_df_open() and decrement after the final
>> vfio_df_close(). This change addresses a KVM reference leak that occurs
>> when a device cdev file is opened multiple times and attempts to bind to
>> iommufd repeatedly.
>>
>> Currently, vfio_df_get_kvm_safe() is invoked prior to each vfio_df_open()
>> in the cdev path during iommufd binding. The corresponding
>> vfio_device_put_kvm() is executed either when iommufd is unbound or if an
>> error occurs during the binding process.
>>
>> However, issues arise when a device binds to iommufd more than once. The
>> second vfio_df_open() will fail during iommufd binding, and
>> vfio_device_put_kvm() will be triggered, setting device->kvm to NULL.
>> Consequently, when iommufd is unbound from the first successfully bound
>> device, vfio_device_put_kvm() becomes ineffective, leading to a leak in the
>> KVM reference count.
>
> To be accurate this happens only when two binds are issued via different
> fds otherwise below check will happen earlier when two binds are in a
> same fd:
>
> /* one device cannot be bound twice */
> if (df->access_granted) {
> ret = -EINVAL;
> goto out_unlock;
> }
yes
>>
>> Below is the calltrace that will be produced in this scenario when the KVM
>> module is unloaded afterwards, reporting "BUG kvm_vcpu (Tainted: G S):
>> Objects remaining in kvm_vcpu on __kmem_cache_shutdown()".
>>
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x80/0xc0
>> slab_err+0xb0/0xf0
>> ? __kmem_cache_shutdown+0xc1/0x4e0
>> ? rcu_is_watching+0x11/0x50
>> ? lock_acquired+0x144/0x3c0
>> __kmem_cache_shutdown+0x1b7/0x4e0
>> kmem_cache_destroy+0xa6/0x260
>> kvm_exit+0x80/0xc0 [kvm]
>> vmx_exit+0xe/0x20 [kvm_intel]
>> __x64_sys_delete_module+0x143/0x250
>> ? ktime_get_coarse_real_ts64+0xd3/0xe0
>> ? syscall_trace_enter+0x143/0x210
>> do_syscall_64+0x6f/0x140
>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> Fixes: 5fcc26969a16 ("vfio: Add VFIO_DEVICE_BIND_IOMMUFD")
>> Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
>> ---
>> drivers/vfio/device_cdev.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
>> index bb1817bd4ff3..3b85d01d1b27 100644
>> --- a/drivers/vfio/device_cdev.c
>> +++ b/drivers/vfio/device_cdev.c
>> @@ -65,6 +65,7 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>> {
>> struct vfio_device *device = df->device;
>> struct vfio_device_bind_iommufd bind;
>> + bool put_kvm = false;
>> unsigned long minsz;
>> int ret;
>>
>> @@ -101,12 +102,15 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>> }
>>
>> /*
>> - * Before the device open, get the KVM pointer currently
>> + * Before the device's first open, get the KVM pointer currently
>> * associated with the device file (if there is) and obtain
>> - * a reference. This reference is held until device closed.
>> + * a reference. This reference is held until device's last closed.
>> * Save the pointer in the device for use by drivers.
>> */
>> - vfio_df_get_kvm_safe(df);
>> + if (device->open_count == 0) {
>> + vfio_df_get_kvm_safe(df);
>> + put_kvm = true;
>> + }
>>
>> ret = vfio_df_open(df);
>> if (ret)
>> @@ -129,7 +133,8 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>> out_close_device:
>> vfio_df_close(df);
>> out_put_kvm:
>> - vfio_device_put_kvm(device);
>> + if (put_kvm)
>> + vfio_device_put_kvm(device);
>> iommufd_ctx_put(df->iommufd);
>> df->iommufd = NULL;
>> out_unlock:
>>
>
> what about extending vfio_df_open() to unify the get/put_kvm()
> and open_count trick in one place?
>
> int vfio_df_open(struct vfio_device_file *df, struct kvm *kvm,
> spinlock_t *kvm_ref_lock)
> {
this should work. But need a comment to note why need pass in both kvm
and kvm_ref_lock given df has both of them. :)
> struct vfio_device *device = df->device;
> int ret = 0;
>
> lockdep_assert_held(&device->dev_set->lock);
>
> if (device->open_count == 0) {
> spin_lock(kvm_ref_lock);
> vfio_device_get_kvm_safe(device, kvm);
> spin_unlock(kvm_ref_lock);
> }
perhaps it can be put in the "if (device->open_count == 1)" branch, just
before invoking vfio_df_device_first_open().
>
> /*
> * Only the group path allows the device to be opened multiple
> * times. The device cdev path doesn't have a secure way for it.
> */
> if (device->open_count != 0 && !df->group)
> return -EINVAL;
>
> device->open_count++;
> if (device->open_count == 1) {
> ret = vfio_df_device_first_open(df);
> if (ret)
> device->open_count--;
> }
>
> if (ret)
> vfio_device_put_kvm(device);
> return ret;
> }
>
> void vfio_df_close(struct vfio_device_file *df)
> {
> struct vfio_device *device = df->device;
>
> lockdep_assert_held(&device->dev_set->lock);
>
> vfio_assert_device_open(device);
> if (device->open_count == 1) {
> vfio_df_device_last_close(df);
> vfio_device_put_kvm(device);
> }
> device->open_count--;
> }
--
Regards,
Yi Liu
Powered by blists - more mailing lists