Re: [PATCH v2 7/8] s390/vfio-ap: Fix required lock not held during display of sysfs status attribute

[Matthew Rosato]

On 8/11/26 2:51 PM, Anthony Krowiak wrote:
> 
> 
> On 8/11/26 2:20 PM, Matthew Rosato wrote:
>> On 8/11/26 2:03 PM, Matthew Rosato wrote:
>>> On 8/10/26 3:22 PM, Anthony Krowiak wrote:
>>>> The status_show function that supports display of the status
>>>> attribute of
>>>> the devices in /sys/bus/ap/devices calls the vfio_ap_mdev_for_queue
>>>> function which iterates the matrix_dev->mdev_list to find the object
>>>> representing the queue device whose status is to be displayed. In
>>>> order to
>>>> traverse this list, the matrix_dev->guests_lock mutex must be held
>>>> which is
>>>> not the case.
>>>>
>>>> To fix this, the guests_lock mutex is taken prior to taking the
>>>> matrix_dev->mdevs_lock mutex in the status_show function. It is taken
>>>> there rather than the vfio_ap_mdev_for_queue function - where it is
>>>> needed - because it must be taken prior to the mdevs_lock mutex in
>>>> order to
>>>> adhere to the proper locking order and prevent a lockdep splat; also
>>>> because the mdevs_lock is needed there to access fields within
>>>> the matrix_mdev object in that function.
>>>>
>>>> Fixes: f139862b92cf ("s390/vfio-ap: add status attribute to AP queue
>>>> device's sysfs dir")
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
>>> Please see my comment on patch 3.
>> Also same idea here, I don't believe the pre-existing finding from
>> Sashiko against this patch is resolved by this series, so have a look
>> and consider a follow-on patch if it's a valid report.
> 
> Can you be more specific as to which Sashiko finding you are talking about?
> 
> 
It only found 1 against this patch.  Quoting:

This is a pre-existing issue, but can this sequence lead to a NULL pointer
dereference if status_show() is called during device probing?
If a userspace process reads the status sysfs attribute before
vfio_ap_mdev_probe_queue() has finished setting the device driver data:
vfio_ap_mdev_probe_queue() {
    ...
    ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
    if (ret)
        return ret;
    q = kzalloc(...);
    ...
    dev_set_drvdata(&apdev->device, q);
}
Userspace could trigger status_show() while the driver data is still NULL.
The dev_get_drvdata() call above would return NULL for q, which is then
passed to vfio_ap_mdev_for_queue() without any checks.
Inside vfio_ap_mdev_for_queue(), the uninitialized q is unconditionally
dereferenced:
vfio_ap_mdev_for_queue() {
    struct ap_matrix_mdev *matrix_mdev;
    unsigned long apid = AP_QID_CARD(q->apqn);
    ...
}
Does the sysfs group creation need to be delayed until after the driver
data is fully initialized and set, or should status_show() check if q is NULL?