On Thu, 19 Aug 2021 09:36:34 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> >>> 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.
>
> >
> > Regards,
> > Halil
> >
> >>
> >>> + 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
> >> [...]
