Re: [PATCH v3 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 8 potential issue(s) to consider:
- [Critical] Refactoring error replaces unlinking with linking, causing a Critical Use-After-Free on mdev removal.
- [High] Unconditional NULL pointer dereference in queue collection causes kernel panic.
- [High] Uninitialized stack-allocated bitmap `apids_removed` causes unintended hardware resets.
- [High] Typo in `reset_removed_queues` skips hardware resets for removed domains, leaking cryptographic state.
- [High] List corruption due to double addition of queues to local reset lists.
- [High] Premature queue linking before matrix validation permanently corrupts hash tables of concurrently running mdevs.
- [High] Domain-only removals completely bypass the hardware reset sequence, leaking cryptographic state.
- [Low] Loop over domain mask uses incorrect limit constant `AP_DEVICES` instead of `AP_DOMAINS`.
--

commit 1cdca5acb48932a1408594014bf5e0f8fa51e334
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Add method to set a new guest AP configuration

Add 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
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -875,7 +896,40 @@ 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]
Does replacing the unlink assignment with vfio_ap_mdev_link_queue() leave
dangling pointers after device teardown?

This function is supposed to unlink the queue from the mdev by setting
q->matrix_mdev = NULL. Actively linking it instead means the queues are never
unlinked, which could cause a use-after-free when the matrix_mdev is
subsequently freed during removal.

[ ... ]

> -static void collect_queues_to_reset(struct ap_matrix_mdev *matrix_mdev,
> -				    unsigned long apid,
> -				    struct list_head *qlist)
> +static void collect_queues_by_apid(struct ap_matrix_mdev *matrix_mdev,
> +				   unsigned long apid,
> +				   struct list_head *qlist)
>  {
>  	struct vfio_ap_queue *q;
>  	unsigned long  apqi;
>  
>  	for_each_set_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm, AP_DOMAINS) {

[Severity: High]
Does this unconditionally dereference matrix_mdev?

The loop condition accesses matrix_mdev->shadow_apcb.aqm without checking if
it is valid. Later code passes a NULL pointer to this function, which will
cause an immediate kernel panic here.

> -		q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi));
> +		q = matrix_mdev ?
> +				vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi)) :
> +				vfio_ap_find_queue(AP_MKQID(apid, apqi));
> +		if (q)
>  			list_add_tail(&q->reset_qnode, qlist);

[Severity: High]
Does appending queues directly to local lists here corrupt the list structure?

If a queue matches both a removed adapter and a removed domain, it is collected
by both collect_queues_by_apid() and collect_queues_by_apqi(). Calling
list_add_tail() unconditionally overwrites the list links of the node,
corrupting the structure of the first list if it wasn't unlinked first.

> +	}
> +}
> +
> +static void collect_queues_by_apqi(struct ap_matrix_mdev *matrix_mdev,
> +				   unsigned long apqi,
> +				   struct list_head *qlist)
> +{
> +	struct vfio_ap_queue *q;
> +	unsigned long  apid;
> +
> +	for_each_set_bit_inv(apid, matrix_mdev->shadow_apcb.apm, AP_DEVICES) {

[Severity: High]
Does this also unconditionally dereference matrix_mdev?

Similar to collect_queues_by_apid(), accessing matrix_mdev->shadow_apcb.apm
here will panic if matrix_mdev is NULL.

[ ... ]

> +static void reset_removed_queues_by_apid(unsigned long *apm_removed,
> +					 unsigned long *apm_filtered,
> +					 struct list_head *qlist)
> +{
> +	DECLARE_BITMAP(apids_removed, AP_DEVICES);

[Severity: High]
Can this uninitialized stack-allocated bitmap cause unintended hardware resets?

DECLARE_BITMAP() allocates the bitmap on the stack without zero-initializing
it. Setting specific bits in it without ever clearing it first means it
retains garbage stack data, which could lead to resetting arbitrary AP
queues across the host.

> +	unsigned long apid;
> +
> +	for_each_set_bit_inv(apid, apm_filtered, AP_DEVICES)
> +		set_bit_inv(apid, apids_removed);
> +
> +	for_each_set_bit_inv(apid, apm_removed, AP_DEVICES) {
> +		if (!test_bit_inv(apid, apids_removed))
> +			set_bit_inv(apid, apids_removed);
> +	}
> +
> +	if (!bitmap_empty(apids_removed, AP_DEVICES)) {
> +		for_each_set_bit_inv(apid, apids_removed, AP_DEVICES)
> +			collect_queues_by_apid(NULL, apid, qlist);

[Severity: High]
Is it intentional to pass NULL as the matrix_mdev here?

As mentioned earlier, collect_queues_by_apid() unconditionally dereferences
this pointer in its loop header, so this call will result in a NULL pointer
dereference.

[ ... ]

> +static void reset_removed_queues(struct ap_matrix *m_removed,
> +				 unsigned long *apm_filtered)
> +{
> +	struct list_head qlist_by_apid, qlist_by_apqi;
> +	DECLARE_BITMAP(apqis, AP_DOMAINS);
> +	unsigned long apqi;
> +
> +	INIT_LIST_HEAD(&qlist_by_apid);
> +	INIT_LIST_HEAD(&qlist_by_apqi);
> +	bitmap_clear(apqis, 0, AP_DOMAINS);
> +
> +	reset_removed_queues_by_apid(m_removed->apm, apm_filtered, &qlist_by_apid);
> +
> +	for_each_set_bit_inv(apqi, m_removed->aqm, AP_DEVICES) {

[Severity: Low]
Should this loop use AP_DOMAINS instead of AP_DEVICES for the limit?

While both constants might currently be defined identically, iterating over the
domain mask (aqm) with the device limit constant could lead to out-of-bounds
accesses if the architecture limits diverge in the future.

> +		set_bit_inv(apqi, apqis);
> +		collect_queues_by_apqi(NULL, apqi, &qlist_by_apqi);
>  	}
>  
> +	if (list_empty(&qlist_by_apqi))
> +		return;
>  
> +	remove_queues_already_reset(&qlist_by_apid, &qlist_by_apqi);
>  
> +	if (!list_empty(&qlist_by_apqi))
> +		vfio_ap_mdev_reset_qlist(&qlist_by_apid);

[Severity: High]
Is there a typo here where qlist_by_apid is being reset instead of
qlist_by_apqi?

The code checks if the apqi list is empty, but then explicitly calls the
reset function on the apid list. This seems to bypass the reset of removed
domain queues entirely, leaving their cryptographic state active.

[ ... ]

> +int vfio_ap_set_new_guest_config(struct ap_matrix_mdev *matrix_mdev,
> +				 struct ap_matrix *m_new,
> +				 bool filtering_allowable)
> +{
> +	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
> +	struct ap_matrix m_old, m_old_shadow, m_removed;
> +	bool do_update;
> +	int rc;
>  
> +	lockdep_assert_held(&ap_attr_mutex);
> +	assert_has_update_locks_for_mdev(matrix_mdev);
> +
> +	/* Save old state */
> +	ap_matrix_copy(&m_old, &matrix_mdev->matrix);
> +	ap_matrix_copy(&m_old_shadow, &matrix_mdev->shadow_apcb);
> +
> +	/* Reset mdev state */
> +	vfio_ap_unlink_queues(matrix_mdev);
> +	ap_matrix_copy(&matrix_mdev->matrix, m_new);
> +	vfio_ap_link_queues(matrix_mdev);
> +
> +	rc = validate_ap_matrix(matrix_mdev);

[Severity: High]
Does linking the queues before validating the matrix corrupt the hash tables
of concurrently running mdevs?

Queues are forcibly linked to the new matrix_mdev's hash table here before
verifying if they are already owned by another mdev in validate_ap_matrix().
If a queue is currently owned by another mdev, hash_add could blindly
overwrite its mdev_qnode pointers, destroying the victim mdev's hash bucket
chain.

> +	if (rc) {
> +		restore_mdev_state(matrix_mdev, &m_old, &m_old_shadow);
> +		return rc;
> +	}
> +
> +	/*
> +	 * If APIDs need to be filtered from the guest AP config and filtering
> +	 * is not allowable according to the caller, then terminate the operation.
> +	 */
>  	do_update = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
> +	if (!bitmap_empty(apm_filtered, AP_DEVICES) && !filtering_allowable) {
> +		restore_mdev_state(matrix_mdev, &m_old, &m_old_shadow);
> +		return -ECANCELED;
> +	}
> +
>  	do_update |= vfio_ap_mdev_filter_cdoms(matrix_mdev);
>  
> +	if (do_update)
>  		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +
> +	get_removed_matrixes(&m_removed, &m_old, m_new);
> +	if (!bitmap_empty(m_removed.apm, AP_DEVICES) ||
> +	    !bitmap_empty(apm_filtered, AP_DEVICES))
> +		reset_removed_queues(&m_removed, apm_filtered);

[Severity: High]
Does this condition bypass the hardware reset sequence for domain-only removals?

If the configuration update only removes AP domains, meaning m_removed.aqm is
populated but m_removed.apm is empty, the entire reset sequence appears to be
skipped. This could leak sensitive cryptographic state to potential subsequent
users.

> +
> +	return 0;
> +}

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