The vfio_ap_mdev_request function in drivers/s390/crypto/vfio_ap_ops.c
accesses fields of an ap_matrix_mdev object without ensuring that the
object is accessed by only one thread at a time. This patch adds the lock
necessary to secure access to the ap_matrix_mdev object.

Fixes: 2e3d8d71e285 ("s390/vfio-ap: wire in the vfio_device_ops request callback")
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Cc: <stable@vger.kernel.org>
---
 drivers/s390/crypto/vfio_ap_ops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a52c2690933f..a2784d3357d9 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2045,6 +2045,7 @@ static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
 	struct ap_matrix_mdev *matrix_mdev;
 
 	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
+	mutex_lock(&matrix_dev->mdevs_lock);
 
 	if (matrix_mdev->req_trigger) {
 		if (!(count % 10))
@@ -2057,6 +2058,8 @@ static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
 		dev_notice(dev,
 			   "No device request registered, blocked until released by user\n");
 	}
+
+	mutex_unlock(&matrix_dev->mdevs_lock);
 }
 
 static int vfio_ap_mdev_get_device_info(unsigned long arg)
-- 
2.47.1

On 2/21/25 10:32 AM, Anthony Krowiak wrote:
> The vfio_ap_mdev_request function in drivers/s390/crypto/vfio_ap_ops.c
> accesses fields of an ap_matrix_mdev object without ensuring that the
> object is accessed by only one thread at a time. This patch adds the lock
> necessary to secure access to the ap_matrix_mdev object.
> 
> Fixes: 2e3d8d71e285 ("s390/vfio-ap: wire in the vfio_device_ops request callback")
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Cc: <stable@vger.kernel.org>

The new code itself seems sane.

But besides this area of code, there are 2 other paths that touch matrix_mdev->req_trigger:

the one via vfio_ap_set_request_irq will already hold the lock via vfio_ap_mdev_ioctl (OK).

However the other one in vfio_ap_mdev_probe acquires mdevs_lock a few lines -after- initializing req_trigger and cfg_chg_trigger to NULL.  Should the lock also be held there since we would have already registered the vfio device above?  We might be protected by circumstance because we are in .probe() but I'm not sure, and we bother getting the lock already to protect mdev_list...

On 3/12/25 11:19 AM, Matthew Rosato wrote:
> On 2/21/25 10:32 AM, Anthony Krowiak wrote:
>> The vfio_ap_mdev_request function in drivers/s390/crypto/vfio_ap_ops.c
>> accesses fields of an ap_matrix_mdev object without ensuring that the
>> object is accessed by only one thread at a time. This patch adds the lock
>> necessary to secure access to the ap_matrix_mdev object.
>>
>> Fixes: 2e3d8d71e285 ("s390/vfio-ap: wire in the vfio_device_ops request callback")
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Cc: <stable@vger.kernel.org>
> The new code itself seems sane.
>
> But besides this area of code, there are 2 other paths that touch matrix_mdev->req_trigger:
>
> the one via vfio_ap_set_request_irq will already hold the lock via vfio_ap_mdev_ioctl (OK).

I have plans to create a patch to insert a call to lockdep_assert_held() 
in functions that require a lock,
such as the one you mentioned

>
> However the other one in vfio_ap_mdev_probe acquires mdevs_lock a few lines -after- initializing req_trigger and cfg_chg_trigger to NULL.  Should the lock also be held there since we would have already registered the vfio device above?  We might be protected by circumstance because we are in .probe() but I'm not sure, and we bother getting the lock already to protect mdev_list...

While it may have been reasonable to include the initialization of those 
fields inside the lock, it really isn't
necessary. These two fields are used to signal events to userspace, so 
these fields will not be used until the
mdev - which is in the process of being created - is attached to a guest 
and the VFIO_DEVICE_SET_IRQS ioctl
is subsequently invoked.

>

PING!


On 2/21/25 10:32 AM, Anthony Krowiak wrote:
> The vfio_ap_mdev_request function in drivers/s390/crypto/vfio_ap_ops.c
> accesses fields of an ap_matrix_mdev object without ensuring that the
> object is accessed by only one thread at a time. This patch adds the lock
> necessary to secure access to the ap_matrix_mdev object.
>
> Fixes: 2e3d8d71e285 ("s390/vfio-ap: wire in the vfio_device_ops request callback")
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Cc: <stable@vger.kernel.org>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a52c2690933f..a2784d3357d9 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2045,6 +2045,7 @@ static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
>   	struct ap_matrix_mdev *matrix_mdev;
>   
>   	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +	mutex_lock(&matrix_dev->mdevs_lock);
>   
>   	if (matrix_mdev->req_trigger) {
>   		if (!(count % 10))
> @@ -2057,6 +2058,8 @@ static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
>   		dev_notice(dev,
>   			   "No device request registered, blocked until released by user\n");
>   	}
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
>   }
>   
>   static int vfio_ap_mdev_get_device_info(unsigned long arg)