| 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
| ||
|
Message-ID: <647c6f6e-33c8-62dd-8f22-c2abafcc5898@linux.ibm.com>
Date: Fri, 20 May 2022 16:59:05 -0400
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: Matthew Rosato <mjrosato@...ux.ibm.com>, jgg@...dia.com,
alex.williamson@...hat.com
Cc: cohuck@...hat.com, borntraeger@...ux.ibm.com,
jjherne@...ux.ibm.com, pasic@...ux.ibm.com,
zhenyuw@...ux.intel.com, zhi.a.wang@...el.com, hch@...radead.org,
intel-gfx@...ts.freedesktop.org,
intel-gvt-dev@...ts.freedesktop.org, linux-s390@...r.kernel.org,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Kevin Tian <kevin.tian@...el.com>,
Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM
On 5/20/22 10:09 AM, Matthew Rosato wrote:
> On 5/20/22 9:56 AM, Tony Krowiak wrote:
>>
>>
>> On 5/19/22 2:33 PM, Matthew Rosato wrote:
>>> Rather than relying on a notifier for associating the KVM with
>>> the group, let's assume that the association has already been
>>> made prior to device_open. The first time a device is opened
>>> associate the group KVM with the device.
>>>
>>> This fixes a user-triggerable oops in GVT.
>>>
>>> Reviewed-by: Tony Krowiak <akrowiak@...ux.ibm.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@...el.com>
>>> Reviewed-by: Christoph Hellwig <hch@....de>
>>> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
>>> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
>>> ---
>>> drivers/gpu/drm/i915/gvt/gtt.c | 4 +-
>>> drivers/gpu/drm/i915/gvt/gvt.h | 3 -
>>> drivers/gpu/drm/i915/gvt/kvmgt.c | 82 ++++++--------------------
>>> drivers/s390/crypto/vfio_ap_ops.c | 35 ++---------
>>> drivers/s390/crypto/vfio_ap_private.h | 3 -
>>> drivers/vfio/vfio.c | 83
>>> ++++++++++-----------------
>>> include/linux/vfio.h | 6 +-
>>> 7 files changed, 57 insertions(+), 159 deletions(-)
>>>
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index e8914024f5b1..a7d2a95796d3 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -1284,25 +1284,6 @@ static void vfio_ap_mdev_unset_kvm(struct
>>> ap_matrix_mdev *matrix_mdev)
>>> }
>>> }
>>> -static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>>> - unsigned long action, void *data)
>>> -{
>>> - int notify_rc = NOTIFY_OK;
>>> - struct ap_matrix_mdev *matrix_mdev;
>>> -
>>> - if (action != VFIO_GROUP_NOTIFY_SET_KVM)
>>> - return NOTIFY_OK;
>>> -
>>> - matrix_mdev = container_of(nb, struct ap_matrix_mdev,
>>> group_notifier);
>>> -
>>> - if (!data)
>>> - vfio_ap_mdev_unset_kvm(matrix_mdev);
>>> - else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
>>> - notify_rc = NOTIFY_DONE;
>>> -
>>> - return notify_rc;
>>> -}
>>> -
>>> static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
>>> {
>>> struct device *dev;
>>> @@ -1402,11 +1383,10 @@ static int vfio_ap_mdev_open_device(struct
>>> vfio_device *vdev)
>>> unsigned long events;
>>> int ret;
>>> - matrix_mdev->group_notifier.notifier_call =
>>> vfio_ap_mdev_group_notifier;
>>> - events = VFIO_GROUP_NOTIFY_SET_KVM;
>>> + if (!vdev->kvm)
>>> + return -EINVAL;
>>> - ret = vfio_register_notifier(vdev, VFIO_GROUP_NOTIFY, &events,
>>> - &matrix_mdev->group_notifier);
>>> + ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
>>> if (ret)
>>> return ret;
>>
>> I'm sorry I didn't see this with my last review, but maybe move the call
>> to vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm) after the successful
>> registration of the IOMMU notifier? This way you won't be plugging AP
>> queues
>> into the guest only to remove them if the registration fails.
>
> This is a pretty edge error case, and the
> vfio_ap_mdev_unset_kvm(matrix_mdev) call at err_kvm should do the
> proper cleanup, right? I guess I'm wondering if it's really any
> different than the prior code which would have registered the
> VFIO_GROUP_NOTIFY_SET_KVM first, which would have immediately
> triggered the notifier since the KVM was already registered to the
> group, meaning it would haved called
> vfio_ap_mdev_group_notifier->vfio_ap_mdev_set_kvm anyway (see
> vfio_register_group_notifier, the "The attaching of kvm and vfio_group
> might already happen..." comment)
You are correct, the VFIO_GROUP_NOTIFY_SET_KVM notifier will get
triggered when it is registered; however, you may have pointed out a
flaw in the previous version of the code. I'm guessing this notifier is
not triggered when it is unregistered, so unless the guest is terminated
due to a non-zero return code from the open_device callback, it will
have access to the AP queues. In hindsight, we probably should have
registered the IOMMU notifier first.
You make a valid point about this being an edge case and I don't think
it's critical, so feel free to keep it as-is.
My r-b still stands.
>
>>
>>> @@ -1415,12 +1395,11 @@ static int vfio_ap_mdev_open_device(struct
>>> vfio_device *vdev)
>>> ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY, &events,
>>> &matrix_mdev->iommu_notifier);
>>> if (ret)
>>> - goto out_unregister_group;
>>> + goto err_kvm;
>>> return 0;
>>> -out_unregister_group:
>>> - vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
>>> - &matrix_mdev->group_notifier);
>>> +err_kvm:
>>> + vfio_ap_mdev_unset_kvm(matrix_mdev);
>>> return ret;
>>> }
>
Powered by blists - more mailing lists