Messages in this thread |  | | Subject | Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback | | From | Tony Krowiak <> | | Date | Tue, 18 May 2021 14:14:03 -0400 |
| |
On 5/18/21 9:59 AM, Christian Borntraeger wrote: > > > On 18.05.21 15:42, Tony Krowiak wrote: >> >> >> On 5/18/21 5:30 AM, Christian Borntraeger wrote: >>> >>> >>> On 17.05.21 21:10, Halil Pasic wrote: >>>> On Mon, 17 May 2021 09:37:42 -0400 >>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>> >>>>>> >>>>>> Because of this, I don't think the rest of your argument is valid. >>>>> >>>>> Okay, so your concern is that between the point in time the >>>>> vcpu->kvm->arch.crypto.pqap_hook pointer is checked in >>>>> priv.c and the point in time the handle_pqap() function >>>>> in vfio_ap_ops.c is called, the memory allocated for the >>>>> matrix_mdev containing the struct kvm_s390_module_hook >>>>> may get freed, thus rendering the function pointer invalid. >>>>> While not impossible, that seems extremely unlikely to >>>>> happen. Can you articulate a scenario where that could >>>>> even occur? >>>> >>>> Malicious userspace. We tend to do the pqap aqic just once >>>> in the guest right after the queue is detected. I do agree >>>> it ain't very likely to happen during normal operation. But why are >>>> you asking? >>> >>> Would it help, if the code in priv.c would read the hook once >>> and then only work on the copy? We could protect that with rcu >>> and do a synchronize rcu in vfio_ap_mdev_unset_kvm after >>> unsetting the pointer? >> >> I'll look into this. > > I think it could work. in priv.c use rcu_readlock, save the > pointer, do the check and call, call rcu_read_unlock. > In vfio_ap use rcu_assign_pointer to set the pointer and > after setting it to zero call sychronize_rcu. > > Halil, I think we can do this as an addon patch as it makes > sense to have this callback pointer protected independent of > this patch. Agree?
I agree that this is a viable option; however, this does not guarantee that the matrix_mdev is not freed thus rendering the function pointer to the interception handler invalid unless that is also included within the rcu_readlock/rcu_read_unlock. That is not possible given the matrix_mdev is freed within the remove callback and the pointer to the structure that contains the interception handler function pointer is cleared in the vfio_ap_mdev_unset_kvm() function. I am working on a patch and should be able to post it before EOD or first thing tomorrow.
|  |