Re: [PATCH v3 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

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

Pre-existing issues:
- [High] Pre-existing TOCTOU race condition in `vfio_ap_mdev_probe_queue` allowing NULL pointer dereference in `status_show`.
--

--- Patch [3]: [PATCH v3 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects ---
commit 431e0fcd6b2e0bbf72d273210e68aa6e049eacc3
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

This commit fixes missing locks needed when traversing or modifying the
ap_matrix_mdev object list in the vfio_ap driver. It ensures guests_lock
is properly held during probe and status reads to prevent concurrent list
access issues.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e382e5a1cb996..5c14ded4fd8ee 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2316,6 +2318,7 @@ static ssize_t status_show(struct device *dev,
>  	struct ap_matrix_mdev *matrix_mdev;
>  	struct ap_device *apdev = to_ap_dev(dev);
>  
> +	mutex_lock(&matrix_dev->guests_lock);
>  	mutex_lock(&matrix_dev->mdevs_lock);
>  	q = dev_get_drvdata(&apdev->device);
>  	matrix_mdev = vfio_ap_mdev_for_queue(q);

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference if the sysfs file is read concurrently during probe?

Looking at vfio_ap_mdev_probe_queue(), the sysfs group is created before
the driver data is initialized and set:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe_queue() {
    ...
    ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
    if (ret)
        return ret;

    q = kzalloc(sizeof(*q), GFP_KERNEL);
    ...
    dev_set_drvdata(&apdev->device, q);
}

If status_show() is called right after the sysfs group is created but
before dev_set_drvdata() is called, dev_get_drvdata() will return NULL.

The NULL pointer is then passed directly to vfio_ap_mdev_for_queue(), which
unconditionally dereferences it:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_for_queue() {
    ...
    unsigned long apid = AP_QID_CARD(q->apqn);
    ...
}

Should there be a check for q being NULL in status_show(), or should the
sysfs group creation be moved after the queue is fully initialized?

>  
>  	/* If the queue is assigned to the matrix mediated device, then
>  	 * determine whether it is passed through to a guest; otherwise,

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