linux-kernel - Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

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]

Date:   Tue, 25 May 2021 13:29:27 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>
Cc:     jjherne@...ux.ibm.com, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, borntraeger@...ibm.com,
        cohuck@...hat.com, pasic@...ux.vnet.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        frankja@...ux.ibm.com, david@...hat.com, imbrenda@...ux.ibm.com,
        hca@...ux.ibm.com
Subject: Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC)
 interception handler

On Tue, May 25, 2021 at 11:56:50AM -0400, Tony Krowiak wrote:

> The vfio_ap_mdev_unset_kvm() function, however, is called both by
> the group notifier when the KVM pointer has been cleared or when the
> mdev is being removed. In both cases, the only way to get the KVM
> pointer - which is needed to unplug the AP resources from the guest
> - is from the matrix_mdev which contains it.

Okay, but that isn't a problem, the matrix dev holds a ref on the kvm
pointer so we can just copy it outside the lock after we prevent it
from changing by unregistering the notifier:

@@ -1362,14 +1365,19 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 {
        struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-       mutex_lock(&matrix_dev->lock);
-       vfio_ap_mdev_unset_kvm(matrix_mdev);
-       mutex_unlock(&matrix_dev->lock);
-
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
                                 &matrix_mdev->iommu_notifier);
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
                                 &matrix_mdev->group_notifier);
+
+       mutex_lock(&matrix_dev->lock);
+       /* matrix_dev->kvm cannot be changed now since we removed the notifiers */
+       kvm = matrix_mdev->kvm;
+       matrix_mdev->kvm = NULL;
+       mutex_unlock(&matrix_dev->lock);
+
+       vfio_ap_mdev_unset_kvm(matrix_mdev, kvm);
+
        module_put(THIS_MODULE);

Note the above misordering is an existing bug too

And reoganize unset_kvm so it uses internal locking and gets the kvm
from the argument.

Also the kvm_busy should be replaced by a proper rwsem, don't try to
open code locks like that - it just defeats lockdep analysis.

Finally, since the only way the ->kvm can be become non-NULL is if the
notifier is registered, release above removes the notifier, and remove
can't be called unless release has been completed, it looks to me like
this the remove check is just dead code, delete it, or leave it as a
WARN_ON:

@@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
        struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);

        mutex_lock(&matrix_dev->lock);
-
-       /*
-        * If the KVM pointer is in flux or the guest is running, disallow
-        * un-assignment of control domain.
-        */
-       if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
-               mutex_unlock(&matrix_dev->lock);
-               return -EBUSY;
-       }

Jason