On Thu, 19 Aug 2021 09:36:34 -0400
Tony Krowiak [off-list ref] wrote:

quoted

quoted

quoted

   static int handle_pqap(struct kvm_vcpu *vcpu)
   {
   	struct ap_queue_status status = {};
+	crypto_hook pqap_hook;
   	unsigned long reg0;
   	int ret;
   	uint8_t fc;

@@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
   	 * Verify that the hook callback is registered, lock the owner
   	 * and call the hook.
   	 */
+	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
   	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
-		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);
+		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;  

Dont we have to check for NULL here? If not can you add a comment why?  

I believe we did the necessary check on the line I just marked with
"<--- HERE".

I find that "*" operator confusing in this context as it doesn't do
any good for us. I believe this situation is described in 6.5.3.2.4 of
the c11 standard. For convenience I will cite from the corresponding
draft:
"The unary * operator denotes indirection. If the operand points to a
function, the result is a function designator; if it points to an
object, the result is an lvalue designating the object. If the operand
has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
invalid value has been assigned to the pointer, the behavior of the
unary * operator is undefined."

Frankly I also fail to see the benefit of introducing the local variable
named "pqap_hook", but back then I decided to not complain about style.  

The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
the reason being that the handle_pqap function in vfio_ap_ops.c
retrieves the matrix_mdev via a container_of macro. The dereferencing
of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
to get the function pointer. There may have been a more stylish
way of doing this, but the functionality is there.

You are right, and I was wrong. But then we do have to distinct pointer
deferences, and we check for NULL only once.

I still do believe we do not have a potential null pointer dereference
here, but the reason for that is that vfio-ap (the party that manages
these pointers) guarantees that whenever
vcpu->kvm->arch.crypto.pqap_hook != NULL is true, 
*vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that
the function pointer is a valid one). Which is the case, because we
set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch
it any more.

In my opinion it is worth a comment.

quoted

Regards,
Halil
 

quoted

quoted

+		ret = pqap_hook(vcpu);

BTW the second dereference takes place here.

If we wanted, we could make sure we don't dereference a null pointer
here but I think that would be an overkill.

Regards,
Halil  

quoted

quoted

[...]