Re: [PATCH] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

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

Pre-existing issues:
- [High] Incorrect bitwise operation `bitmap_andnot` used instead of `bitmap_and` for control domains in `vfio_ap_mdev_cfg_remove`, preventing removal of unplugged domains.
--

--- Patch [1]: [PATCH] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
commit 7ae73def648a068715a8cdb4bf574ac5755f8e77
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

This commit adds NULL pointer checks for matrix_mdev->kvm prior to taking
the kvm->lock mutex in the configuration change and scan complete callbacks.
This prevents a NULL pointer dereference when the matrix mdev has no
associated KVM instance.

Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..a0b7c37fee281 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2606,7 +2606,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	int do_remove = 0;
>  
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> -		mutex_lock(&matrix_mdev->kvm->lock);
> +		/*
> +		 * If the mdev is attached to a KVM guest, we will need to
> +		 * hold the KVM lock in order to update the guest's AP
> +		 * configuration if any adapters, domains or control domains
> +		 * have been removed.
> +		 */
> +		if (matrix_mdev->kvm)
> +			mutex_lock(&matrix_mdev->kvm->lock);
> +
>  		mutex_lock(&matrix_dev->mdevs_lock);
>  
>  		do_remove |= bitmap_and(aprem, ap_remove,

[Severity: High]
This is a pre-existing issue, but I noticed a potential logic flaw just
below this context in vfio_ap_mdev_cfg_remove() within
drivers/s390/crypto/vfio_ap_ops.c.

Does the control domain removal incorrectly use bitmap_andnot() instead of
bitmap_and()?

When a host administrator or hypervisor unplugs a control domain, the code
evaluates the removed domains using:

	do_remove |= bitmap_andnot(cdrem, cd_remove,
				   matrix_mdev->matrix.adm,
				   AP_DOMAINS);

Since this evaluates to cd_remove & ~matrix_mdev->matrix.adm, doesn't this
mean assigned domains that were removed from the host will resolve to 0?

Could this prevent explicitly unplugged control domains from being removed
from the KVM guest, allowing it to retain access to hardware it shouldn't
have?

[ ... ]

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