On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger [off-list ref] wrote:

On 18.05.21 15:42, Tony Krowiak wrote:

quoted

On 5/18/21 5:30 AM, Christian Borntraeger wrote:  

quoted

On 17.05.21 21:10, Halil Pasic wrote:  

quoted

On Mon, 17 May 2021 09:37:42 -0400
Tony Krowiak [off-list ref] wrote:
 

quoted

quoted

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?

Unfortunately just "the hook" is ambiguous in this context. We
have kvm->arch.crypto.pqap_hook that is supposed to point to
a struct kvm_s390_module_hook member of struct ap_matrix_mdev 
which is also called pqap_hook. And struct kvm_s390_module_hook
has function pointer member named "hook".

quoted

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.

In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hook pointer properly synchronized. I'm
not sure if that is what you are proposing. How do we usually
do synchronisation on the stuff that lives in kvm->arch?

BTW, something as simple as a cmpxchg which boils down to the
CSG instruction for us would suffice in this case (or forcing
any interlocked update type construct). 

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?

Unfortunately I didn't quite get at the bottom of what exactly gets
leaked. My intuition is, that trading a leak for an use after free
is in general not a good idea. In this particular case, assuming
userspace is well behaved, the use after free is very unlikely,
but then I don't consider the leak to be awfully likely either. A
well behaved userspace should not attempt to remove the mdev while
it is associated with a guest. We documented that in:
Documentation/s390/vfio-ap.rst
"""
  remove:
    deallocates the mediated matrix device's ap_matrix_mdev structure. This will
    be allowed only if a running guest is not using the mdev.
"""
BTW this patch should probably change that piece of documentation as
well.

In this case, because the leak is much likelier than the use after
free (assuming a non-malicious-userspace) the trade may be worth it. Yet my
independent opinion is that I would prefer this fixed in one go and
properly. But I do trust your judgement better than mine (especially in
matters like this). So feel free to go ahead (i.e. I'm not going to NACK
this). 

Regards,
Halil