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

On 8/11/26 2:55 PM, Matthew Rosato wrote:
> 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: [email protected]
>>>>> Signed-off-by: Anthony Krowiak <[email protected]>
>>>> 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?

I have a fix for this, shall I include it in this series or send it as 
an individual
patch?

>