Re: [PATCH v5 09/15] s390/vfio-ap: Add method to set a new guest AP configuration

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] vfio_ap_mdev_unlink_fr_queues incorrectly links instead of unlinking queues, causing a Use-After-Free when an mdev is removed.

Pre-existing issues:
- [Critical] Queues filtered from the guest APCB do not have their matrix_mdev pointer cleared during hot unplug, leading to a dangling pointer UAF.
--

--- Patch [9]: [PATCH v5 09/15] s390/vfio-ap: Add method to set a new guest AP configuration ---
commit c2bb55ba8d50e1fc1f6fb1459cd3cfcecf5e78d5
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Add method to set a new guest AP configuration
    
Adds a new vfio_ap_set_new_config function to set a guest's AP
configuration. This is needed in order to set the state of the mdev when
it is migrated from a remote host system during the RESUMING phase.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 678bdd4c82e95..454bb9fe19716 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -875,7 +827,7 @@ static void vfio_ap_mdev_unlink_fr_queues(struct ap_matrix_mdev *matrix_mdev)
>  			q = vfio_ap_mdev_get_queue(matrix_mdev,
>  						   AP_MKQID(apid, apqi));
>  			if (q)
> -				q->matrix_mdev = NULL;
> +				vfio_ap_mdev_link_queue(matrix_mdev, q);

[Severity: Critical]
Is it intended to link the queue here rather than nullifying the pointer?

When an admin removes an AP mdev device, vfio_ap_mdev_remove() calls
vfio_ap_mdev_unlink_fr_queues() immediately before freeing the matrix_mdev.

Since this replaces the pointer nullification (q->matrix_mdev = NULL) with
a call to vfio_ap_mdev_link_queue(), which acts as a no-op if the queue is
already linked, does this leave the physical queues with dangling pointers
to the freed mdev?

>  		}
>  	}
>  }

[ ... ]

> @@ -1725,54 +1678,100 @@ static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix *src)

[ ... ]

> +int vfio_ap_set_new_guest_config(struct ap_matrix_mdev *matrix_mdev,
> +				 struct ap_matrix *m_new)
> +{

[ ... ]

> +	/* Need old bitmaps in matrix_mdev for unplug/unlink */
> +	ap_matrix_copy(&matrix_mdev->matrix, &m_old);
> +
> +	/* Unlink removed adapters/domains */
> +	vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, m_removed.apm);

[Severity: Critical]
This is a pre-existing issue, but can this lead to a use-after-free for
queues that are filtered from the guest APCB?

If an admin unassigns an adapter that was filtered from shadow_apcb,
vfio_ap_mdev_hot_unplug_adapters() unlinks it from the hash table but skips
adding the filtered queue to qlist (checked in vfio_ap_mdev_unlink_adapter).

Because it is never added to qlist, the cleanup loop in
vfio_ap_mdev_hot_unplug_adapters():

list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
        vfio_ap_unlink_mdev_fr_queue(q);
        list_del(&q->reset_qnode);
}

completely misses this queue. This leaves q->matrix_mdev pointing to the
mdev. Since the adapter is cleared from matrix_mdev->matrix.apm, a later
mdev destruction would skip cleaning it up, leaving a dangling pointer.

> +	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, m_removed.aqm);
> +
> +	/* Need new bitmaps in matrix_mdev for linking new adapters/domains */
> +	ap_matrix_copy(&matrix_mdev->matrix, m_new);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724161351.1802644-1-akrowiak@linux.ibm.com?part=9