Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback

[Halil Pasic <pasic@linux.ibm.com>]

On Thu, 13 May 2021 10:35:05 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 5/12/21 2:35 PM, Halil Pasic wrote:
> > On Mon, 10 May 2021 17:48:37 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >  
> >> The mdev remove callback for the vfio_ap device driver bails out with
> >> -EBUSY if the mdev is in use by a KVM guest. The intended purpose was
> >> to prevent the mdev from being removed while in use; however, returning a
> >> non-zero rc does not prevent removal. This could result in a memory leak
> >> of the resources allocated when the mdev was created. In addition, the
> >> KVM guest will still have access to the AP devices assigned to the mdev
> >> even though the mdev no longer exists.
> >>
> >> To prevent this scenario, cleanup will be done - including unplugging the
> >> AP adapters, domains and control domains - regardless of whether the mdev
> >> is in use by a KVM guest or not.
> >>
> >> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Tony Krowiak <akrowiak@stny.rr.com>
> >> ---
> >>   drivers/s390/crypto/vfio_ap_ops.c | 13 ++-----------
> >>   1 file changed, 2 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >> index b2c7e10dfdcd..f90c9103dac2 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -26,6 +26,7 @@
> >>
> >>   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> >>   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
> >> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev);
> >>
> >>   static int match_apqn(struct device *dev, const void *data)
> >>   {
> >> @@ -366,17 +367,7 @@ 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;
> >> -	}
> >> -
> >> -	vfio_ap_mdev_reset_queues(mdev);
> >> +	vfio_ap_mdev_unset_kvm(matrix_mdev);
> >>   	list_del(&matrix_mdev->node);
> >>   	kfree(matrix_mdev);  
> > Are we at risk of handle_pqap() in arch/s390/kvm/priv.c using an
> > already freed pqap_hook (which is a member of the matrix_mdev pointee
> > that is freed just above my comment).
> >
> > I'm aware of the fact that vfio_ap_mdev_unset_kvm() does a
> > matrix_mdev->kvm->arch.crypto.pqap_hook = NULL but that is
> > AFRICT not done under any lock relevant for handle_pqap(). I guess
> > the idea is, I guess, the check cited below
> >
> > static int handle_pqap(struct kvm_vcpu *vcpu)
> > [..]
> >          /*
> >           * Verify that the hook callback is registered, lock the owner
> >           * and call the hook.
> >           */
> >          if (vcpu->kvm->arch.crypto.pqap_hook) {
> >                  if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> >                          return -EOPNOTSUPP;
> >                  ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> >                  module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> >                  if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
> >                          kvm_s390_set_psw_cc(vcpu, 3);
> >                  return ret;
> >          }
> >
> > is going to catch it, but I'm not sure it is guaranteed to catch it.
> > Opinions?  
> 
> The hook itself - handle_pqap() function in vfio_ap_ops.c - also checks
> to see if the reference to the hook is set and terminates with an error 
> if it
> is not. If the hook is invoked subsequent to the remove callback above,
> all should be fine since the check is also done under the matrix_dev->lock.
> 

I don't quite understand your logic. Let us assume matrix_mdev was freed,
but vcpu->kvm->arch.crypto.pqap_hook still points to what used to be
(*matrix_mdev).pqap_hook. In that case the function pointer
vcpu->kvm->arch.crypto.pqap_hook->hook is used after it was freed, and
may not point to the handle_pqap() function in vfio_ap_ops.c, thus the
check you are referring to ain't necessarily relevant. Than is
if you mean the check in the  handle_pqap() function in vfio_ap_ops.c; if
you mean the check in handle_pqap() in arch/s390/kvm/priv.c, that one is
not done under the matrix_dev->lock. Or do I have a hole somewhere in my
reasoning?

Regards,
Halil