linux-kernel - Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c1ec6ce7-45fe-3fac-8e77-25f40b2a6cad@linux.ibm.com>
Date:   Mon, 23 Aug 2021 09:08:37 -0400
From:   Tony Krowiak <akrowiak@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        cohuck@...hat.com, pasic@...ux.vnet.ibm.com, jjherne@...ux.ibm.com,
        jgg@...dia.com, alex.williamson@...hat.com, kwankhede@...dia.com,
        david@...hat.com
Subject: Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler
 function pointer



On 8/19/21 5:42 PM, Halil Pasic wrote:
> On Thu, 19 Aug 2021 09:36:34 -0400
> Tony Krowiak <akrowiak@...ux.ibm.com> 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.

Even I had to look at it again to respond to you, so a comment
is probably called for.

>
>
>>> 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.

I agree, it is overkill.

>
> Regards,
> Halil
>>>> [...]