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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch attempts to fix a TOCTOU race by holding update locks during sysfs group creation, but leaves the exact same race window open on the allocation error path, leading to a NULL pointer dereference.
- [High] Missing flush or cancellation of `q->reset_work` in `vfio_ap_mdev_remove_queue` leads to a Use-After-Free.
--

--- Patch [8]: [PATCH v4 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe ---
commit 9b9dde366169d761a8888f77c5299cbff3f3b6ea
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix NULL deref in status_show() during queue probe

This commit acquires the update locks before calling sysfs_create_group()
during queue probe to prevent a NULL pointer dereference in a concurrent
read of the status attribute. It also optimizes APQN retrieval.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -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);
>  	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
>  	return ret;

[Severity: High]
This is a pre-existing issue, but does releasing the update locks before
removing the sysfs group reopen the race window on the allocation error path?

If kzalloc_obj() fails, the error path releases guests_lock before calling
sysfs_remove_group(). This immediately unblocks any concurrent readers of
status_show() while the sysfs file still exists.

Once unblocked, status_show() will retrieve NULL from dev_get_drvdata() and
pass it to vfio_ap_mdev_for_queue(), which unconditionally dereferences it:

vfio_ap_ops.c:status_show()
    q = dev_get_drvdata(&apdev->device);
    matrix_mdev = vfio_ap_mdev_for_queue(q);

vfio_ap_ops.c:vfio_ap_mdev_for_queue()
    unsigned long apid = AP_QID_CARD(q->apqn);

Should sysfs_remove_group() be called before release_update_locks_for_mdev()
here to prevent this NULL pointer dereference?

> +
> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
> +	return ret;
>  }
>  
>  void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  {

[ ... ]

>  	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
>  	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
>  		vfio_ap_mdev_reset_queue(q);
>  		flush_work(&q->reset_work);
>  	} else {
>  		vfio_ap_free_aqic_resources(q);
>  	}
>  
>  done:
>  	if (matrix_mdev)
>  		vfio_ap_unlink_queue_fr_mdev(q);
>  
>  	dev_set_drvdata(&apdev->device, NULL);
>  	kfree(q);

[Severity: High]
This is a pre-existing issue, but does this conditional bypass of
flush_work() create a use-after-free risk?

If a hardware hot-unplug races with a background reset operation, and the
queue is not in the host's AP configuration, the teardown path skips flushing
the reset_work and frees the queue object.

Meanwhile, the background worker continues executing and accesses the freed
structure:

vfio_ap_ops.c:apq_reset_check()
    q = container_of(reset_work, struct vfio_ap_queue, reset_work);
    memcpy(&status, &q->reset_status, sizeof(status));
    ...
    status = ap_tapq(q->apqn, NULL);

Should flush_work() or cancel_work_sync() be called unconditionally before
kfree(q) to prevent this memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8