Re: [PATCH] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Restoring `kvm->arch.crypto.pqap_hook` to `&m->pqap_hook` after releasing update locks creates a TOCTOU race condition leading to an exploitable Use-After-Free.
--

--- Patch [1]: [PATCH] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
commit 3eedf7c518752f33e7b7b7758dc2098b54d797b9
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()

This commit addresses an issue where the pqap_hook pointer is left pointing
at a failing matrix_mdev when a conflicting assignment is detected. It fixes
this by restoring the hook to the legitimately attached mdev discovered
during the error path.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..e18540fd6dc75 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -1833,6 +1839,10 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>  		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
>  			if (m != matrix_mdev && m->kvm == kvm) {
>  				release_update_locks_for_kvm(kvm);
> +				/* Undo the hook installation since we're failing */
> +				down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> +				kvm->arch.crypto.pqap_hook = &m->pqap_hook;

[Severity: Critical]
Is there a race condition introduced here since the update locks are released
right before restoring the hook pointer?

Looking at vfio_ap_mdev_set_kvm() in drivers/s390/crypto/vfio_ap_ops.c, 
release_update_locks_for_kvm() drops the mutexes that protect the mdev
list. Once those locks are dropped, could another thread concurrently remove
and free the existing mdev (m)?

If m is freed while we are waiting to acquire the pqap_hook_rwsem, this code
would then assign a dangling pointer to kvm->arch.crypto.pqap_hook. Later,
when the guest executes a PQAP instruction, it might dereference this freed
memory, resulting in a use-after-free.

Would it be safer to restore the hook pointer before calling
release_update_locks_for_kvm() to ensure m remains valid?

> +				up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>  				return -EPERM;
>  			}
>  		}

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