Re: [PATCH v5 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

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

Pre-existing issues:
- [High] Leaked pinned NIB memory and KVM GISC registration in `vfio_ap_irq_enable()` error path.
- [High] Leaked `req_trigger` and `cfg_chg_trigger` eventfd contexts on VFIO device close/remove.
--

--- Patch [6]: [PATCH v5 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap ---
commit 6362e7fffee6cfc724530d3fd54b669440496a88
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

The DECLARE_BITMAP(apm_filtered, AP_DEVICES) macro allocates the bitmap
on the stack without zero-initializing it. This can cause unintended
guest hardware queue resets if vfio_ap_mdev_filter_matrix() is bypassed.
This patch zero-initializes the apm_filtered bitmap at the beginning of
vfio_ap_mdev_hot_plug_cfg() using bitmap_zero().

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 0f3537aadea87..d667ad4bbf705 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2861,6 +2861,15 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>  	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>  	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
>  
> +	/*
> +	 * Zero out the apm_filtered bitmap in case there are no adapters or
> +	 * domains to be added, but only control domains. In that case,
> +	 * vfio_ap_mdev_filter_matrix() - which initializes apm_filtered - will
> +	 * not get called and the reset_queues_for_apids will crash because it
> +	 * will access an uninitialized bitmap.
> +	 */
> +	bitmap_zero(apm_filtered, AP_DEVICES);
> +
>  	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
>  					    matrix_mdev->apm_add, AP_DEVICES);
>  	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,

[ ... ]

> 	case AP_RESPONSE_OTHERWISE_CHANGED:
> 		/* We could not modify IRQ settings: clear new configuration */
> 		ret = kvm_s390_gisc_unregister(kvm, isc);
> 		if (ret)
> 			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
> 					 __func__, ret, isc, q->apqn);
> 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
> 		break;
> 	default:
> 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> 			status.response_code);
> 		vfio_ap_irq_disable(q);
> 		break;
> 	}

[Severity: High]
This is a pre-existing issue, but does this default error path in
vfio_ap_irq_enable() leak the newly allocated NIB pages and KVM GISC
registration?

If a user-space process configures device IRQs and triggers
vfio_ap_irq_enable(), the function successfully pins the new NIB and
registers the GISC. If ap_aqic() fails with an unexpected status code, it
falls through to this default case.

Since vfio_ap_irq_disable(q) only cleans up previously saved resources in
q->saved_iova and q->saved_isc, it appears the newly pinned nib and isc
local variables are left abandoned.

[ ... ]

> static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
> {
> 	struct ap_matrix_mdev *matrix_mdev =
> 		container_of(vdev, struct ap_matrix_mdev, vdev);
> 
> 	vfio_ap_mdev_unset_kvm(matrix_mdev);
> }

[Severity: High]
This is a pre-existing issue, but it looks like this teardown path might
leak the req_trigger and cfg_chg_trigger eventfd contexts.

When a user-space process sets up IRQs, eventfd_ctx_fdget() takes a
reference on the eventfds. If the process crashes or closes the VFIO device
file without explicitly clearing the IRQs, shouldn't
vfio_ap_mdev_close_device() drop these references with eventfd_ctx_put()?

[ ... ]

> static void vfio_ap_mdev_remove(struct mdev_device *mdev)
> {
> 	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(&mdev->dev);
> 
> 	vfio_unregister_group_dev(&matrix_mdev->vdev);
> 
> 	mutex_lock(&matrix_dev->guests_lock);
> 	mutex_lock(&matrix_dev->mdevs_lock);
> 	vfio_ap_mdev_reset_queues(matrix_mdev);
> 	vfio_ap_mdev_unlink_fr_queues(matrix_mdev);
> 	list_del(&matrix_mdev->node);
> 	mutex_unlock(&matrix_dev->mdevs_lock);
> 	mutex_unlock(&matrix_dev->guests_lock);
> 	vfio_put_device(&matrix_mdev->vdev);
> }

[Severity: High]
This is a pre-existing issue, but similar to the above, is it possible for
the mdev teardown path in vfio_ap_mdev_remove() to free the device
structure without cleaning up the eventfd context references?

Could repeatedly opening and closing the device exhaust system file
descriptors and kernel memory due to these missing eventfd_ctx_put()
calls?

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