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

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

Pre-existing issues:
- [High] Missing synchronization for asynchronous reset work during device removal leads to Use-After-Free.
--

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

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

This commit fixes a null pointer dereference in status_show() by checking
if the driver data has been set before proceeding. It also adjusts lock
acquisition during queue probe and simplifies the apqn assignment.

Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 16779cfc64e87..1edd0b7a3cce7 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2472,8 +2492,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;
> +
> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
> +	return ret;
>  }
>  
>  void vfio_ap_mdev_remove_queue(struct ap_device *apdev)

[Severity: High]
This is a pre-existing issue, but looking at the rest of 
vfio_ap_mdev_remove_queue() below this signature, is there a potential
use-after-free if the queue is removed from the host config?

Further down in vfio_ap_mdev_remove_queue():

	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);

If an administrator hot-unplugs the ap adapter concurrently with a guest
triggering a reset, the queue is no longer in the host's configuration.

Because flush_work(&q->reset_work) is inside the conditional block, it is
skipped in this scenario. The function then proceeds directly to kfree(q).

Could the asynchronous reset worker thread (apq_reset_check) then execute
and dereference the freed q object, resulting in a use-after-free?

Should flush_work(&q->reset_work) be moved outside the condition to ensure
any pending reset work completes before the queue memory is freed?

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