Re: [PATCH v5 06/56] scsi: qla2xxx: Remove redundant VPD flash read in sysfs read path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless calls to `qla28xx_get_aux_images` and `qla27xx_get_active_image` cause data races on hardware flash MMIO registers.
- [Medium] Lockless read of shared buffer `ha->vpd` via `memory_read_from_buffer` allows torn reads from concurrent modifications.
--

--- Patch [6]: [PATCH v5 06/56] scsi: qla2xxx: Remove redundant VPD flash read in sysfs read path ---
commit 58886d8609178ebe2a5d0c1f553bf284e06082ab
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Remove redundant VPD flash read in sysfs read path

This commit removes a redundant flash read from the qla2x00_sysfs_read_vpd
sysfs read path. The second read executed without holding the optrom_mutex,
exposing flash access to concurrent operations.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 6a05ce195aa05..800751ab562a1 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -580,7 +580,6 @@ qla2x00_sysfs_read_vpd(struct file *filp, struct kobject *kobj,
>  	ha->isp_ops->read_optrom(vha, ha->vpd, faddr, ha->vpd_size);
>  	mutex_unlock(&ha->optrom_mutex);
>  
> -	ha->isp_ops->read_optrom(vha, ha->vpd, faddr, ha->vpd_size);
>  skip:
>  	return memory_read_from_buffer(buf, count, &off, ha->vpd, ha->vpd_size);

[Severity: Medium]
This is a pre-existing issue, but does dropping the optrom_mutex right
before calling memory_read_from_buffer() expose the shared ha->vpd buffer
to torn reads?

If another privileged thread calls qla2x00_sysfs_write_vpd() concurrently,
it acquires optrom_mutex and overwrites the shared ha->vpd buffer.

Because memory_read_from_buffer() executes without the lock here, it might
copy a concurrently mutating buffer to userspace.

>  }

[Severity: High]
This is another pre-existing issue, but can the earlier lockless call to
qla28xx_get_aux_images() cause hardware data races?

The qla28xx_get_aux_images() function is called before optrom_mutex is
acquired in qla2x00_sysfs_read_vpd():

drivers/scsi/qla2xxx/qla_attr.c:qla2x00_sysfs_read_vpd() {
        ...
        if (IS_QLA28XX(ha)) {
                qla28xx_get_aux_images(vha, &active_regions);
        ...
        mutex_lock(&ha->optrom_mutex);
        ...
}

This eventually calls qla24xx_read_flash_dword(), which issues hardware
writes to reg->flash_addr and polling reads from reg->flash_data.

Concurrently executing another flash operation interleaves these hardware
register accesses and can irreparably corrupt the flash operation state
machine for both threads.

A similar lockless access happens in the BSG handler
qla2x00_get_flash_image_status() when it calls qla27xx_get_active_image().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717152904.3155730-1-njavali@marvell.com?part=6