Re: [PATCH v4 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 8/12/26 1:58 PM, Matthew Rosato wrote:
> On 8/12/26 1:09 PM, Anthony Krowiak wrote:
>> When vfio_ap_mdev_probe_queue() creates the sysfs attribute group,
>> the queue's driver data has not yet been set. A concurrent read of
>> the 'status' attribute can therefore call dev_get_drvdata() and
>> get NULL, which is then passed directly to
>> vfio_ap_mdev_for_queue() where q->apqn is unconditionally
>> dereferenced, causing a NULL pointer dereference.
>>
>> Fix this by acquiring the update locks before calling
>> sysfs_create_group(). The status_show() function acquires
>> guests_lock before reading the driver data, so any concurrent
>> read will block until after dev_set_drvdata() has been called
>> and the update locks are released.
>>
>> As a bonus, the APQN no longer needs to be read from the queue
>> struct after allocation — it can be read directly from apdev
>> before allocation and stored in a local variable, which is then
>> assigned to q->apqn once the allocation succeeds.
>>
>> Fixes: 260f3ea141382 ("s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 16779cfc64e8..b0454a296c67 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2424,14 +2424,17 @@ void vfio_ap_mdev_unregister(void)
>>   
>>   int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   {
>> -	int ret;
>> +	int ret, apqn;
>>   	struct vfio_ap_queue *q;
>>   	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>>   	struct ap_matrix_mdev *matrix_mdev;
>>   
>> +	apqn = to_ap_queue(&apdev->device)->qid;
>> +	matrix_mdev = get_update_locks_by_apqn(apqn);
>> +
>>   	ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
>>   	if (ret)
>> -		return ret;
>> +		goto err_release_locks;
>>   
>>   	q = kzalloc_obj(*q);
>>   	if (!q) {
>> @@ -2439,11 +2442,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   		goto err_remove_group;
>>   	}
>>   
>> -	q->apqn = to_ap_queue(&apdev->device)->qid;
>> +	q->apqn = apqn;
>>   	q->saved_isc = VFIO_AP_ISC_INVALID;
>>   	memset(&q->reset_status, 0, sizeof(q->reset_status));
>>   	INIT_WORK(&q->reset_work, apq_reset_check);
>> -	matrix_mdev = get_update_locks_by_apqn(q->apqn);
>>   
>>   	if (matrix_mdev) {
>>   		vfio_ap_mdev_link_queue(matrix_mdev, q);
>> @@ -2472,8 +2474,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   	return ret;
>>   
>>   err_remove_group:
>> +	release_update_locks_for_mdev(matrix_mdev);
> I think now you've just shrunk the window instead of totally closed it?

Since that locks must be released before the call to sysfs_remove_group()
to avoid an ABBA deadlock, it is not possible to totally close it.

>
> Can't sysfs still be accessed between this release and the remove_group?

That possibility is incredibly remote, but I suppose anything is possible.

>
> Do you think adding an additional NULL check against dev_get_drvdata()
> in status_show() and treating the NULL case like the other error
> conditions would be enough to cover that window?

It would cover that window, but that function does not return any error, so
we'd have to decide what makes the most sense between the following
three values, or create new one to indicate we don't know the status.

AP_QUEUE_ASSIGNED
AP_QUEUE_UNASSIGNED
AP_QUEUE_IN_USE

>   
>
>
>