Skip to contentSkip to conversation

scsi: qla2xxx: Bug fixes and hardening

PATCH Applied Nilesh Javali 2 participants 35 messages Jul 30 – Aug 7

Subsystems (2)
lore
Share & tools
lore

Nilesh Javali

author maintainer
#0 lore raw

cover scsi: qla2xxx: Bug fixes and hardening

This series collects bug fixes, hardening, and small cleanups for the qla2xxx driver that are independent of the QLA29xx adapter enablement. Most were uncovered by static analysis and fuzzing of the driver's interrupt, mailbox, NVMe, and BSG paths; 30 of the 33 patches carry a
Fixes: tag and are marked for stable.

The series is organised as follows:

Queue pairs, MSI-X, and interrupt setup/teardown
Clamp MSI-X derived queue counts to avoid truncation, fix a use-after-free of qpair work on queue teardown, and quiesce the response IRQ before freeing the request queue.

Firmware dump, FCE trace, and flash/version paths
Improve firmware dump data capture, serialize the flash version read in the reset handler, clarify the MPI optrom address/length units, fix FCE trace enable parsing in debugfs, and fix a use-after-free of the FCE trace during a firmware dump.

Probe and mailbox paths
Fix the cs84xx use-after-free on host teardown, don't query firmware state while the chip is down, zero the mailbox struct in qla2x00_get_firmware_state(), and null out freed pointers in the qla2x00_mem_alloc() error path.

Response/status IOCB path
Use memset_io() to clear the QLAFX00 request ring slot, fix response queue over-consumption in __qla_consume_iocb(), fix a soft lockup in the polling continuation IOCB signature, bound rsp_info_len to avoid an out-of-bounds sense-data read, avoid a req_q_map double-read in qla2x00_error_entry(), and reject non-SCSI SRBs on the status IOCB fast path.

NPIV and report-ID acquisition
Clamp max_npiv_vports to the VP_CTRL bitmap capacity, avoid a double completion on async IOCB timeout, and correct vport handling in report ID acquisition (skip a vport under deletion, drop the vport reference under lock, and hold vport_slock for the host map update).

NVMe LS and abort handling
Fix an abort reference leak on repeated abort, skip the NVMe LS reject IOCB when firmware is not started, unlink the unsolicited context before freeing on the LS reject error path, and serialize the unsolicited context list with a per-fcport lock.

BSG passthrough hardening
Use a coherent DMA buffer for D_Port diagnostics, zero-init bsg stack buffers and the SFP DMA buffer to avoid information leaks, validate the BSG request_len before reading vendor_cmd[], and bound i2c->length in the I2C bsg handlers.

The final patch bumps the driver version to 12.00.00.2607b2.

The series applies on top of the qla2xxx QLA29xx series (v6, 56 patches) on Linux 7.2-rc1.

Thanks,
Nilesh

Nilesh Javali (32):
scsi: qla2xxx: Clamp MSI-X derived queue counts to avoid truncation
scsi: qla2xxx: Serialize flash version read in reset handler
scsi: qla2xxx: Fix use-after-free of qpair work on queue teardown
scsi: qla2xxx: Clarify MPI optrom address/length units
scsi: qla2xxx: Fix cs84xx use-after-free on host teardown
scsi: qla2xxx: Don't query firmware state while chip is down
scsi: qla2xxx: Zero mailbox struct in qla2x00_get_firmware_state()
scsi: qla2xxx: Fix FCE trace enable parsing in debugfs
scsi: qla2xxx: Fix FCE trace use-after-free during firmware dump
scsi: qla2xxx: Use memset_io() to clear QLAFX00 request ring slot
scsi: qla2xxx: Null out freed pointers in qla2x00_mem_alloc() error
path
scsi: qla2xxx: Fix response queue over-consumption in

    __qla_consume_iocb()

scsi: qla2xxx: Fix soft lockup polling continuation IOCB signature
scsi: qla2xxx: Bound rsp_info_len to avoid OOB sense-data read
scsi: qla2xxx: Avoid req_q_map double-read in qla2x00_error_entry()
scsi: qla2xxx: Quiesce response IRQ before freeing request queue
scsi: qla2xxx: Reject non-SCSI SRB on status IOCB fast path
scsi: qla2xxx: Clamp max_npiv_vports to VP_CTRL bitmap capacity
scsi: qla2xxx: Avoid double completion in async IOCB timeout
scsi: qla2xxx: Skip vport under deletion in report ID acquisition
scsi: qla2xxx: Drop vport reference under lock in report ID
acquisition
scsi: qla2xxx: Hold vport_slock for host map update in report ID
acquisition
scsi: qla2xxx: Fix NVMe abort reference leak on repeated abort
scsi: qla2xxx: Skip NVMe LS reject IOCB when FW not started
scsi: qla2xxx: Unlink NVMe unsol ctx before freeing on LS reject error
scsi: qla2xxx: Serialize NVMe unsol ctx list with a per-fcport lock
scsi: qla2xxx: Use coherent DMA buffer for D_Port diagnostics
scsi: qla2xxx: Zero-init bsg stack buffers to avoid info leak
scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]
scsi: qla2xxx: Zero SFP DMA buffer in FRU/I2C bsg handlers
scsi: qla2xxx: Bound i2c->length in I2C bsg handlers
scsi: qla2xxx: Update version to 12.00.00.2607b2

Quinn Tran (1):
scsi: qla2xxx: Improve firmware dump data capture

 drivers/scsi/qla2xxx/qla_attr.c    |   6 +-
 drivers/scsi/qla2xxx/qla_bsg.c     |  89 ++++++++++---
 drivers/scsi/qla2xxx/qla_dbg.c     |   6 +-
 drivers/scsi/qla2xxx/qla_def.h     |   5 +
 drivers/scsi/qla2xxx/qla_dfs.c     |   4 +-
 drivers/scsi/qla2xxx/qla_fw.h      |   4 +
 drivers/scsi/qla2xxx/qla_init.c    | 195 ++++++++++++++++++++---------
 drivers/scsi/qla2xxx/qla_inline.h  |  13 ++
 drivers/scsi/qla2xxx/qla_iocb.c    |   4 +-
 drivers/scsi/qla2xxx/qla_isr.c     |  72 ++++++++---
 drivers/scsi/qla2xxx/qla_mbx.c     |  23 ++--
 drivers/scsi/qla2xxx/qla_mid.c     |   6 +-
 drivers/scsi/qla2xxx/qla_nvme.c    |  35 ++++--
 drivers/scsi/qla2xxx/qla_os.c      |  37 ++++--
 drivers/scsi/qla2xxx/qla_sup.c     |   5 +-
 drivers/scsi/qla2xxx/qla_tmpl.c    |  48 +++++--
 drivers/scsi/qla2xxx/qla_version.h |   2 +-
 17 files changed, 415 insertions(+), 139 deletions(-)

base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
prerequisite-patch-id: 66f1e857c5c541b44c44c84f0435f5ae694cd352
prerequisite-patch-id: a10016425fce97adb39b7c9f4e16957eb9fa2bbf
prerequisite-patch-id: 5638c8253ef25fdbdb693f187ebf4aca164a53bb
prerequisite-patch-id: 5fd0239f192c5a70b72fd792f963b35279586bf3
prerequisite-patch-id: d670db315c2f7616da067a9c598c026d3d02a72c
prerequisite-patch-id: 089e6ca8556eff71f73e4b0b3b38e227a64e7a8a
prerequisite-patch-id: d9fd72003f9c0f97a0a767352f85ff437423c0dd
prerequisite-patch-id: b537af431e09ba282ddbbab7de129a4df96462bf
prerequisite-patch-id: ef008cebf8c927f411895f21a93c9069b333b652
prerequisite-patch-id: d6ff17f825e353b2caea629baee1cbc58e16e5e7
prerequisite-patch-id: d2038244d4a8cce08f259193041b967cb4ef4ca6
prerequisite-patch-id: 1bf075ed572822ab7579ad2b7c4aab4c032fcb17
prerequisite-patch-id: 2a8b1b21a31bf85364cd7073e536d3020fc5c4ce
prerequisite-patch-id: 16f0e4175b832f16429f5de87a066de70895f779
prerequisite-patch-id: ad1e5c20ae62c396770122eb009a97b412c08e84
prerequisite-patch-id: 8dc8524dd7e3da7c97f4522c51ed478b79bd0a60
prerequisite-patch-id: 291dfd641efce49ca7ab983c5b8c64febfe307ca
prerequisite-patch-id: 064bf1f6e6da602f8356a7ee35b481a275890d96
prerequisite-patch-id: af35378d8f9b58e3ffa080ac27357ce2a4b2aeec
prerequisite-patch-id: 8623dc9b2d9ab9d07ced7fa9f4c5c649e3f036c8
prerequisite-patch-id: 52ed04f7a9c3cac327fa0235fab899335a128363
prerequisite-patch-id: ef93f470d241eaa9e86f4db1421b99687d22f5e6
prerequisite-patch-id: 3a19f5ba3798c0bbc7047070a45e7a62877319b4
prerequisite-patch-id: 6da0949a6775445497bbace21164ae15041bc4b6
prerequisite-patch-id: 0b301f1d06ca9975230e435c613d2c4f7f78d02e
prerequisite-patch-id: 2ac2f4c5f34d759a701ffea19e2f69a68ef83351
prerequisite-patch-id: 2966bec1fb19ff8d36bcd699d48e8cb3ed2e4e21
prerequisite-patch-id: 75ace74de11917bb5a11b1c42563ddd9db594f08
prerequisite-patch-id: f12a610ede731a7d35a5ea0f860f78f833b2c6b0
prerequisite-patch-id: 96ccc66d0453f29a69b2244173a7561c661919f6
prerequisite-patch-id: 5d204a6aabec64593b4e3bd79d741d550a03b8cf
prerequisite-patch-id: 06e52cc37105e4151f0a56ea2b4a9f5cfe3e4c7b
prerequisite-patch-id: c0d0d2f057ecdd0312f680f5b75b11edf1034a27
prerequisite-patch-id: 7573db59b84ad4e1a1fd9961cff0ed62f2420383
prerequisite-patch-id: 08725130a6f128f6619eca4e326b7f79b2c001a6
prerequisite-patch-id: 268c107f1e71cc790154939fe98dee493a74a6f7
prerequisite-patch-id: e071b465300f26cf0fa73d1c5ea5714a410b8698
prerequisite-patch-id: d73b83500639494fce110c269fd2a4586a79ef59
prerequisite-patch-id: 30a4e9270bab0316d0951ad49516b8ee27866c3f
prerequisite-patch-id: 17b5595c786875e0baafad2d72efb780698ba7fd
prerequisite-patch-id: f532ad2a90127ff323d1042ffe627f96d50ba024
prerequisite-patch-id: 56a601420cbe264384b37abdff28438c64055769
prerequisite-patch-id: 7b8df4b25162742447d879cbc70c514ed07a59d1
prerequisite-patch-id: eb9c50d47882057aac8ebbdf4db5cd4708881c37
prerequisite-patch-id: b13cb77c2941d6a914e0590f5334197a59cc8efb
prerequisite-patch-id: f29d36b73d29cafab251da268bfa24d2619db7c4
prerequisite-patch-id: 5c835f539357a54f92321a8f8bc3e0231b0cf875
prerequisite-patch-id: 54d1b521971e052e15a8adf7d804d2de725afc4a
prerequisite-patch-id: bdbf9290d186914aab1f1995cb1d517ebca803c4
prerequisite-patch-id: 6d3c080e7078f612b28319f0c4c5e432bea99262
prerequisite-patch-id: ccdc3ed2515919d51f497d2933d002fa78923e30
prerequisite-patch-id: 853f939f5dfd7d3a82e7a673ba6a8a231b6ba84a
prerequisite-patch-id: c04850b87d8122792e919840bdafb46a7baa8998
prerequisite-patch-id: 8e3485fdc5e43c883f080315e875fc525c053082
prerequisite-patch-id: 72833e5cfa0e8b92edabb760243b6c5eb119e8e4
prerequisite-patch-id: dcaccf685f03d118656accaae9811df505ab7b4a

Signature · 1 line
2.47.3
author maintainer
#1 lore raw

PATCH 1/33 scsi: qla2xxx: Clamp MSI-X derived queue counts to avoid truncation

ha->msix_count is u16, but ha->max_req_queues, ha->max_rsp_queues and ha->max_qpairs are u8. Deriving the queue count as "ha->max_req_queues = ha->msix_count - 1" therefore truncates: a board (or a misconfigured/malicious hot-plugged device) advertising 257 MSI-X vectors yields msix_count - 1 == 256, which truncates to 0. An MSI-X count of 1 zeroes it as well, and in target mode the subsequent "ha->max_req_queues--" then underflows 0 to 255.

When the count is 0, qla2x00_alloc_queues() calls kzalloc_objs(struct req_que *, 0), which returns ZERO_SIZE_PTR. That is not NULL, so the allocation check passes and the following "ha->req_q_map[0] = req" dereferences ZERO_SIZE_PTR, corrupting memory or crashing the kernel.

Add qla_calc_queue_count() to clamp the derived value into [1, QLA_MAX_QUEUES - 1] so it always fits in u8 and is never zero, and use it at all three derivation sites (qla25xx_iospace_config(), qla83xx_iospace_config() and qla24xx_enable_msix()). Also guard the target-mode decrement so it cannot reintroduce a zero (which would in turn underflow max_qpairs).

Fixes d74595278f4a ("scsi: qla2xxx: Add multiple queue pair functionality.")
---
 drivers/scsi/qla2xxx/qla_inline.h | 13 +++++++++++++
 drivers/scsi/qla2xxx/qla_isr.c    |  4 ++--
 drivers/scsi/qla2xxx/qla_os.c     |  6 +++---
 3 files changed, 18 insertions(+), 5 deletions(-)
drivers/scsi/qla2xxx/qla_inline.h +13 -0
index 9e33bcc87b39..90da4bad8e6e 100644
@@ -138,6 +138,19 @@ qla_logio_set_vp_index(struct qla_hw_data *ha, void *pkt, u16 vp_idx)
 		((struct logio_entry_24xx *)pkt)->vp_index = vp_idx;
 }
 
+static inline u8
+qla_calc_queue_count(u16 msix_count)
+{
+	/*
+	 * Request/response queues are bounded by the MSI-X vector count less
+	 * the mailbox vector.  These counters are u8, so a board advertising
+	 * e.g. 257 vectors would truncate msix_count - 1 (256) to 0 and hand
+	 * kzalloc_objs() a zero count (ZERO_SIZE_PTR), faulting on the first
+	 * ha->req_q_map[0] store.  Clamp into [1, QLA_MAX_QUEUES - 1].
+	 */
+	return clamp_t(u16, msix_count - 1, 1, QLA_MAX_QUEUES - 1);
+}
+
 static inline void
 qla2x00_poll(struct rsp_que *rsp)
 {
drivers/scsi/qla2xxx/qla_isr.c +2 -2
index 9d2d11f10a76..86e1e8428f2a 100644
@@ -4705,10 +4705,10 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
 		ha->msix_count = ret;
 		/* Recalculate queue values */
 		if (ha->mqiobase && (ql2xmqsupport || ql2xnvmeenable)) {
-			ha->max_req_queues = ha->msix_count - 1;
+			ha->max_req_queues = qla_calc_queue_count(ha->msix_count);
 
 			/* ATIOQ needs 1 vector. That's 1 less QPair */
-			if (QLA_TGT_MODE_ENABLED())
+			if (QLA_TGT_MODE_ENABLED() && ha->max_req_queues > 1)
 				ha->max_req_queues--;
 
 			ha->max_rsp_queues = ha->max_req_queues;
drivers/scsi/qla2xxx/qla_os.c +3 -3
index 186c6c7a3944..5209dda45459 100644
@@ -2132,7 +2132,7 @@ qla2x00_iospace_config(struct qla_hw_data *ha)
 		ha->msix_count = msix + 1;
 		/* Max queues are bounded by available msix vectors */
 		/* MB interrupt uses 1 vector */
-		ha->max_req_queues = ha->msix_count - 1;
+		ha->max_req_queues = qla_calc_queue_count(ha->msix_count);
 		ha->max_rsp_queues = ha->max_req_queues;
 		/* Queue pairs is the max value minus the base queue pair */
 		ha->max_qpairs = ha->max_rsp_queues - 1;
@@ -2224,10 +2224,10 @@ qla83xx_iospace_config(struct qla_hw_data *ha)
 		 */
 		if (ql2xmqsupport || ql2xnvmeenable) {
 			/* MB interrupt uses 1 vector */
-			ha->max_req_queues = ha->msix_count - 1;
+			ha->max_req_queues = qla_calc_queue_count(ha->msix_count);
 
 			/* ATIOQ needs 1 vector. That's 1 less QPair */
-			if (QLA_TGT_MODE_ENABLED())
+			if (QLA_TGT_MODE_ENABLED() && ha->max_req_queues > 1)
 				ha->max_req_queues--;
 
 			ha->max_rsp_queues = ha->max_req_queues;
Signature · 1 line
2.47.3
author maintainer
#2 lore raw

PATCH 2/33 scsi: qla2xxx: Improve firmware dump data capture

In reply to Nilesh Javali

From: Quinn Tran <[email protected]>

Capture as much firmware dump data as possible. Save the mailbox registers at start-of-day, before firmware execution, so they are available in the dump, and allocate a guestimate dump buffer early during driver load to capture failures that happen before the final dump buffer is sized.

Make template entry processing more robust: skip over any entry that fails to capture and continue with the next one, and skip entries that time out instead of aborting the whole dump. Notify udev once sysfs nodes are available in case a dump was captured before they existed.

---
 drivers/scsi/qla2xxx/qla_dbg.c  |   4 +-
 drivers/scsi/qla2xxx/qla_def.h  |   3 +
 drivers/scsi/qla2xxx/qla_init.c | 120 +++++++++++++++++---------------
 drivers/scsi/qla2xxx/qla_os.c   |   8 +++
 drivers/scsi/qla2xxx/qla_tmpl.c |  48 ++++++++++---
 5 files changed, 116 insertions(+), 67 deletions(-)
drivers/scsi/qla2xxx/qla_dbg.c +2 -2
index acb58daacf35..4f756468ea64 100644
@@ -172,7 +172,7 @@ qla27xx_dump_mpi_ram(struct qla_hw_data *ha, uint32_t addr, uint32_t *ram,
 
 		if (!test_and_clear_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags)) {
 			/* no interrupt, timed out*/
-			return rval;
+			return QLA_FUNCTION_TIMEOUT;
 		}
 		if (rval) {
 			/* error completion status */
@@ -255,7 +255,7 @@ qla24xx_dump_ram(struct qla_hw_data *ha, uint32_t addr, __be32 *ram,
 
 		if (!test_and_clear_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags)) {
 			/* no interrupt, timed out*/
-			return rval;
+			return QLA_FUNCTION_TIMEOUT;
 		}
 		if (rval) {
 			/* error completion status */
drivers/scsi/qla2xxx/qla_def.h +3 -0
index c10414453c2d..2684d785ecbf 100644
@@ -4176,6 +4176,7 @@ struct qla_hw_data {
 #define SRB_MIN_REQ     128
 	mempool_t       *srb_mempool;
 	u8 port_name[WWN_SIZE];
+	u16 mbregs[32];
 
 	volatile struct {
 		uint32_t	mbox_int		:1;
@@ -4246,6 +4247,8 @@ struct qla_hw_data {
 		uint32_t	eeh_flush:2;
 #define EEH_FLUSH_RDY  1
 #define EEH_FLUSH_DONE 2
+		uint32_t	t262_fail:1;
+		uint32_t	t272_fail:1;
 		uint32_t	secure_mcu:1;
 		uint32_t	valid_flt:1;
 	} flags;
drivers/scsi/qla2xxx/qla_init.c +64 -56

192 lines not on this page · raw message

drivers/scsi/qla2xxx/qla_os.c +8 -0
index 5209dda45459..62c9bd0fe06d 100644
@@ -3721,6 +3721,14 @@ qla2x00_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (test_bit(UNLOADING, &base_vha->dpc_flags))
 		return -ENODEV;
 
+	/*
+	 * FW dump can happens before sysfs nodes are created.  If sysfs nodes
+	 * are unavailable then udev script will not be able to read the fw dump.
+	 * Notify udev to read again, now that sysfs nodes are available.
+	 */
+	if (ha->fw_dumped || ha->mpi_fw_dumped)
+		qla2x00_post_uevent_work(base_vha, QLA_UEVENT_CODE_FW_DUMP);
+
 	return 0;
 
 probe_failed:
drivers/scsi/qla2xxx/qla_tmpl.c +39 -9

82 lines not on this page · raw message

Signature · 1 line
2.47.3
author maintainer
#3 lore raw

PATCH 3/33 scsi: qla2xxx: Serialize flash version read in reset handler

In reply to Nilesh Javali

The "update cache versions without reset" sysfs reset operation (0x20261) calls get_flash_version(), which reads hardware flash registers, without holding ha->optrom_mutex. The VPD update path serializes the same call under optrom_mutex, so this reset path can interleave its flash register accesses with a concurrent VPD or optrom flash operation and corrupt the reads.

Hold ha->optrom_mutex across the get_flash_version() call to match the VPD update path.

Fixes 8c2cf7d4e387 ("[SCSI] qla2xxx: Add a new interface to update versions.")
---
 drivers/scsi/qla2xxx/qla_attr.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/qla2xxx/qla_attr.c +2 -0
index 3b24e8a5e29b..6a87d3bb0b0e 100644
@@ -815,7 +815,9 @@ qla2x00_sysfs_write_reset(struct file *filp, struct kobject *kobj,
 			    "Unable to allocate memory for VPD information update.\n");
 			return -ENOMEM;
 		}
+		mutex_lock(&ha->optrom_mutex);
 		ha->isp_ops->get_flash_version(vha, tmp_data);
+		mutex_unlock(&ha->optrom_mutex);
 		vfree(tmp_data);
 		break;
 	}
Signature · 1 line
2.47.3
author maintainer
#4 lore raw

PATCH 4/33 scsi: qla2xxx: Fix use-after-free of qpair work on queue teardown

In reply to Nilesh Javali

The response queue MSI-X handler qla2xxx_msix_rsp_q() schedules qla_do_work() via queue_work(ha->wq, &qpair->q_work). qla_do_work() dereferences the qpair (vha, rsp) and takes qpair->qp_lock.

During teardown, qla2xxx_delete_qpair() deletes the response queue, which calls free_irq() in qla25xx_free_rsp_que(), and then frees the queue and the qpair. free_irq() waits for running hardirq handlers but does not cancel work already placed on ha->wq. A still-pending q_work then runs qla_do_work() against the freed qpair and response queue, causing a use-after-free. This is especially likely during full adapter teardown, where destroy_workqueue(ha->wq) forces pending work to run after the queue pairs have been freed.

Flush the work item with cancel_work_sync() in qla25xx_free_rsp_que() after free_irq() has released the interrupt (so no new work can be queued) and before the response queue and qpair memory are freed (so the flushed handler still sees valid memory). Guard on rsp->qpair and ha->wq to match the INIT_WORK() condition and avoid operating on an uninitialized work_struct.

Fixes 68ca949cdb04 ("[SCSI] qla2xxx: Add CPU affinity support.")
---
 drivers/scsi/qla2xxx/qla_mid.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/scsi/qla2xxx/qla_mid.c +4 -0
index b7d9c1a53f3c..33bfc61d8165 100644
@@ -606,6 +606,10 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
 		rsp->msix->handle = NULL;
 	}
 
+	/* Flush any queued response work before freeing the queue/qpair. */
+	if (rsp->qpair && ha->wq)
+		cancel_work_sync(&rsp->qpair->q_work);
+
 	if (rsp->ring)
 		dma_free_coherent(&ha->pdev->dev,
 				  (rsp->length + 1) * rsp_entry_size,
Signature · 1 line
2.47.3
author maintainer
#5 lore raw

PATCH 5/33 scsi: qla2xxx: Clarify MPI optrom address/length units

In reply to Nilesh Javali

The kdoc for qla29xx_mpi_optrom_data() described @offset as an "Offset into the device memory", which reads like a byte address and invites confusion with the per-chunk word-granular address advance in the transfer loop.

MBC_LOAD_DUMP_MPI_RAM is word-addressed: @offset is an MPI RAM address in 32-bit words, and @length is a byte count that is converted internally to a word count. Document this to reflect the existing behavior. No functional change.

---
 drivers/scsi/qla2xxx/qla_sup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/scsi/qla2xxx/qla_sup.c +3 -2
index 2e5fde403442..56ef2b4c7c26 100644
@@ -560,8 +560,9 @@ static void set_chunk_mpi_bits(uint16_t *options, int count, int total)
  * @vha: Pointer to SCSI QLogic host structure.
  * @opts: Options for the operation.
  * @buf: Buffer to read from/write to.
- * @offset: Offset into the device memory.
- * @length: Length of data, in bytes.
+ * @offset: MPI RAM address, in 32-bit words (MBC_LOAD_DUMP_MPI_RAM is
+ *	    word-addressed; not a byte offset).
+ * @length: Length of data, in bytes (converted internally to a word count).
  * @op: Operation, either QLA29XX_MPI_OP_DUMP or QLA29XX_MPI_OP_LOAD.
  *
  * Returns:
Signature · 1 line
2.47.3
author maintainer
#6 lore raw

PATCH 6/33 scsi: qla2xxx: Fix cs84xx use-after-free on host teardown

In reply to Nilesh Javali

qla84xx_put_chip() drops the last reference to ha->cs84xx and frees it via __qla84xx_chip_release() without clearing ha->cs84xx. During teardown it ran before scsi_remove_host(), which is what removes the 84xx_fw_version host sysfs attribute. A concurrent read of that attribute in the window between the two calls executes qla24xx_84xx_fw_version_show(), which dereferences the freed ha->cs84xx, resulting in a use-after-free.

Move qla84xx_put_chip() to after scsi_remove_host() in both qla2x00_remove_one() and qla2x00_disable_board_on_pci_error(). Once scsi_remove_host() returns, the sysfs attribute is gone and kernfs has drained any in-flight show(), so no reader can touch cs84xx; the put still runs before the host and ha are freed.

Fixes fe1b806f4f71 ("[SCSI] qla2xxx: Refactor shutdown code so some functionality can be reused.")
---
 drivers/scsi/qla2xxx/qla_os.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/scsi/qla2xxx/qla_os.c +4 -4
index 62c9bd0fe06d..4f485e4acf4a 100644
@@ -4048,8 +4048,6 @@ qla2x00_remove_one(struct pci_dev *pdev)
 
 	qla2x00_dfs_remove(base_vha);
 
-	qla84xx_put_chip(base_vha);
-
 	/* Disable timer */
 	if (base_vha->timer_active)
 		qla2x00_stop_timer(base_vha);
@@ -4074,6 +4072,8 @@ qla2x00_remove_one(struct pci_dev *pdev)
 
 	scsi_remove_host(base_vha->host);
 
+	qla84xx_put_chip(base_vha);
+
 	qla2x00_free_device(base_vha);
 
 	qla2x00_clear_drv_active(ha);
@@ -6995,8 +6995,6 @@ qla2x00_disable_board_on_pci_error(struct work_struct *work)
 
 	qla2x00_dfs_remove(base_vha);
 
-	qla84xx_put_chip(base_vha);
-
 	if (base_vha->timer_active)
 		qla2x00_stop_timer(base_vha);
 
@@ -7014,6 +7012,8 @@ qla2x00_disable_board_on_pci_error(struct work_struct *work)
 
 	scsi_remove_host(base_vha->host);
 
+	qla84xx_put_chip(base_vha);
+
 	base_vha->flags.init_done = 0;
 	qla25xx_delete_queues(base_vha);
 	qla2x00_free_fcports(base_vha);
Signature · 1 line
2.47.3
author maintainer
#7 lore raw

PATCH 7/33 scsi: qla2xxx: Don't query firmware state while chip is down

In reply to Nilesh Javali

qla2x00_fw_state_show() initializes rval to QLA_FUNCTION_FAILED and jumps to the out: label when the chip is down or EEH is busy. The out: block then re-issued qla2x00_get_firmware_state() because rval != QLA_SUCCESS, defeating the chip-down/EEH-busy guards and issuing a mailbox command (outside optrom_mutex) during ISP reset or PCI error recovery, which can hang the adapter. It also turned a normal in-lock mailbox failure into a second unsynchronized mailbox attempt.

Make the out: fallback only mark the firmware state as unknown. The mailbox is now issued at most once, inside optrom_mutex, and only when the chip is up and not EEH-busy.

Fixes b6faaaf796d7 ("scsi: qla2xxx: Serialize mailbox request")
---
 drivers/scsi/qla2xxx/qla_attr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/scsi/qla2xxx/qla_attr.c +1 -3
index 6a87d3bb0b0e..a4ca22024ede 100644
@@ -1678,10 +1678,8 @@ qla2x00_fw_state_show(struct device *dev, struct device_attribute *attr,
 	rval = qla2x00_get_firmware_state(vha, state);
 	mutex_unlock(&vha->hw->optrom_mutex);
 out:
-	if (rval != QLA_SUCCESS) {
+	if (rval != QLA_SUCCESS)
 		memset(state, -1, sizeof(state));
-		rval = qla2x00_get_firmware_state(vha, state);
-	}
 
 	return scnprintf(buf, PAGE_SIZE, "0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n",
 	    state[0], state[1], state[2], state[3], state[4], state[5]);
Signature · 1 line
2.47.3
author maintainer
#8 lore raw

PATCH 8/33 scsi: qla2xxx: Zero mailbox struct in qla2x00_get_firmware_state()

In reply to Nilesh Javali

The mbx_cmd_t is allocated on the stack but left uninitialized. qla2x00_mailbox_command() has several early-return paths (PCI permanent failure, device failed, EEH busy, ISP abort pending, mailbox access timeout, purge mbox) that return without writing the input mailbox registers back into mcp->mb[]. qla2x00_get_firmware_state() then unconditionally copies mcp->mb[1..6] (and mb[12]) into the caller's states[] array regardless of the return value.

On such a failure the copied values are uninitialized kernel stack memory, which is then exposed to userspace via the fw_state and mpi_fw_state sysfs handlers. Zero the mailbox struct so a failed query yields deterministic zeroed state instead of leaking stack contents.

Fixes 4d4df1932b6b ("[SCSI] qla2xxx: Add ISP84XX support.")
---
 drivers/scsi/qla2xxx/qla_mbx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/qla2xxx/qla_mbx.c +2 -0
index ba4a4764de1f..ab5648eb5f20 100644
@@ -2276,6 +2276,8 @@ qla2x00_get_firmware_state(scsi_qla_host_t *vha, uint16_t *states)
 	if (!ha->flags.fw_started)
 		return QLA_FUNCTION_FAILED;
 
+	memset(&mc, 0, sizeof(mc));
+
 	mcp->mb[0] = MBC_GET_FIRMWARE_STATE;
 	mcp->out_mb = MBX_0;
 	if (IS_FWI2_CAPABLE(vha->hw))
Signature · 1 line
2.47.3
author maintainer
#9 lore raw

PATCH 9/33 scsi: qla2xxx: Fix FCE trace enable parsing in debugfs

In reply to Nilesh Javali

qla2x00_dfs_fce_write() called kstrtoul() with a NULL result pointer, so a successful parse would dereference NULL and oops. Worse, the int return value (0 on success, negative errno on failure) was assigned to the unsigned long enable flag, inverting the intended logic: a valid number was treated as "disable" while a parse failure enabled FCE.

Parse the value into enable and propagate parse errors to userspace.

Fixes 841df27d619e ("scsi: qla2xxx: Move FCE Trace buffer allocation to user control")
---
 drivers/scsi/qla2xxx/qla_dfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_dfs.c +3 -1
index 177d47e92e49..5d08bdbcf70a 100644
@@ -510,7 +510,9 @@ qla2x00_dfs_fce_write(struct file *file, const char __user *buffer,
 		return PTR_ERR(buf);
 	}
 
-	enable = kstrtoul(buf, 0, 0);
+	rc = kstrtoul(buf, 0, &enable);
+	if (rc)
+		goto out_free;
 	rc = count;
 
 	mutex_lock(&ha->fce_mutex);
Signature · 1 line
2.47.3
author maintainer
#10 lore raw

PATCH 10/33 scsi: qla2xxx: Fix FCE trace use-after-free during firmware dump

In reply to Nilesh Javali

qla2x00_free_fce_trace() freed and cleared ha->fce while holding only fce_mutex. The firmware-dump consumers qla27xx_fwdt_entry_t264() and qla25xx_copy_fce() read ha->fce (NULL check followed by a copy of the buffer) under hardware_lock and never take fce_mutex. A debugfs FCE disable could therefore free the DMA buffer between a dump's NULL check and its copy, resulting in a use-after-free.

Unpublish ha->fce under hardware_lock, then release the lock and free the DMA buffer (dma_free_coherent() may sleep). A concurrent dump either completes its check and copy with the buffer still valid, or observes ha->fce == NULL and skips it.

Fixes 841df27d619e ("scsi: qla2xxx: Move FCE Trace buffer allocation to user control")
---
 drivers/scsi/qla2xxx/qla_init.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)
drivers/scsi/qla2xxx/qla_init.c +18 -2
index fb417364fa75..fed6dbc3b6ae 100644
@@ -3752,11 +3752,27 @@ int qla2x00_alloc_fce_trace(scsi_qla_host_t *vha)
 
 void qla2x00_free_fce_trace(struct qla_hw_data *ha)
 {
-	if (!ha->fce)
+	void *fce;
+	dma_addr_t fce_dma;
+	unsigned long flags;
+
+	/*
+	 * Unpublish ha->fce under hardware_lock so a firmware dump in
+	 * progress (which reads ha->fce under the same lock) cannot race
+	 * with the buffer being freed.
+	 */
+	spin_lock_irqsave(&ha->hardware_lock, flags);
+	if (!ha->fce) {
+		spin_unlock_irqrestore(&ha->hardware_lock, flags);
 		return;
-	dma_free_coherent(&ha->pdev->dev, FCE_SIZE, ha->fce, ha->fce_dma);
+	}
+	fce = ha->fce;
+	fce_dma = ha->fce_dma;
 	ha->fce = NULL;
 	ha->fce_dma = 0;
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
+
+	dma_free_coherent(&ha->pdev->dev, FCE_SIZE, fce, fce_dma);
 }
 
 static void
Signature · 1 line
2.47.3
author maintainer
#11 lore raw

PATCH 11/33 scsi: qla2xxx: Use memset_io() to clear QLAFX00 request ring slot

In reply to Nilesh Javali

For QLAFX00 the request ring is ioremapped device I/O memory (ha->iobase + req_que_off), not DMA-coherent RAM, which is why the rest of the FX00 path accesses it through memcpy_toio() and the wrt_reg_* helpers. __qla2x00_alloc_iocbs() however zeroed the producer slot with a plain memset(). On architectures such as ARM64 a regular memset() may emit unaligned or block-zeroing instructions (e.g. DC ZVA) that are invalid on Device memory, leading to a synchronous external abort.

Use memset_io() to clear the slot for QLAFX00, matching the I/O accessors used elsewhere on this ring. Other adapters keep the plain memset() on their DMA-coherent rings. The zero-fill is retained for FX00 because its IOCB builders (e.g. qlafx00_fxdisc_iocb()) copy only part of the entry and rely on the unused tail being pre-zeroed.

Fixes 8ae6d9c7eb10 ("[SCSI] qla2xxx: Enhancements to support ISPFx00.")
---
 drivers/scsi/qla2xxx/qla_iocb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_iocb.c +3 -1
index c4595626c16b..88bae1166a24 100644
@@ -2530,11 +2530,13 @@ __qla2x00_alloc_iocbs(struct qla_qpair *qpair, srb_t *sp)
 	 */
 	req->cnt -= req_cnt;
 	pkt = qla_req_ring_slot(ha, req);
-	memset(pkt, 0, qla_req_entry_size(ha));
 	if (IS_QLAFX00(ha)) {
+		memset_io((void __iomem __force *)pkt, 0,
+			  qla_req_entry_size(ha));
 		wrt_reg_byte((u8 __force __iomem *)&pkt->entry_count, req_cnt);
 		wrt_reg_dword((__le32 __force __iomem *)&pkt->handle, handle);
 	} else {
+		memset(pkt, 0, qla_req_entry_size(ha));
 		pkt->entry_count = req_cnt;
 		pkt->handle = handle;
 	}
Signature · 1 line
2.47.3
author maintainer
#12 lore raw

PATCH 12/33 scsi: qla2xxx: Null out freed pointers in qla2x00_mem_alloc() error path

In reply to Nilesh Javali

When qla2x00_mem_alloc() fails, qla2x00_probe_one() jumps to probe_hw_failed and calls qla2x00_mem_free(). Several error labels in qla2x00_mem_alloc() freed adapter members (elsrej.c, purex_dma_pool, flt, sfp_data, loop_id_map, async_pd, sf_init_cb, ex_init_cb, npiv_info) but left the pointers dangling. qla2x00_mem_free() then freed them a second time. Worse, for the dma_pool members it issued dma_pool_free(ha->s_dma_pool, ...) after s_dma_pool had already been destroyed and set to NULL at fail_s_dma_pool, dereferencing a NULL pool.

Clear each freed pointer (and its DMA handle) in the error labels so the subsequent qla2x00_mem_free() skips them.

---
 drivers/scsi/qla2xxx/qla_os.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/scsi/qla2xxx/qla_os.c +15 -0
index 4f485e4acf4a..918b00aed8b8 100644
@@ -4619,28 +4619,43 @@ qla2x00_mem_alloc(struct qla_hw_data *ha, uint16_t req_len, uint16_t rsp_len,
 fail_lsrjt:
 	dma_free_coherent(&ha->pdev->dev, ha->elsrej.size,
 			  ha->elsrej.c, ha->elsrej.cdma);
+	ha->elsrej.c = NULL;
+	ha->elsrej.cdma = 0;
 fail_elsrej:
 	dma_pool_destroy(ha->purex_dma_pool);
+	ha->purex_dma_pool = NULL;
 fail_flt_data:
 	vfree(ha->flt_data);
 	ha->flt_data = NULL;
 fail_flt:
 	dma_free_coherent(&ha->pdev->dev, sizeof(struct qla_flt_header) + FLT_REGIONS_SIZE,
 	    ha->flt, ha->flt_dma);
+	ha->flt = NULL;
+	ha->flt_dma = 0;
 
 fail_flt_buffer:
 	dma_free_coherent(&ha->pdev->dev, SFP_DEV_SIZE,
 	    ha->sfp_data, ha->sfp_data_dma);
+	ha->sfp_data = NULL;
+	ha->sfp_data_dma = 0;
 fail_sfp_data:
 	kfree(ha->loop_id_map);
+	ha->loop_id_map = NULL;
 fail_loop_id_map:
 	dma_pool_free(ha->s_dma_pool, ha->async_pd, ha->async_pd_dma);
+	ha->async_pd = NULL;
+	ha->async_pd_dma = 0;
 fail_async_pd:
 	dma_pool_free(ha->s_dma_pool, ha->sf_init_cb, ha->sf_init_cb_dma);
+	ha->sf_init_cb = NULL;
+	ha->sf_init_cb_dma = 0;
 fail_sf_init_cb:
 	dma_pool_free(ha->s_dma_pool, ha->ex_init_cb, ha->ex_init_cb_dma);
+	ha->ex_init_cb = NULL;
+	ha->ex_init_cb_dma = 0;
 fail_ex_init_cb:
 	kfree(ha->npiv_info);
+	ha->npiv_info = NULL;
 fail_npiv_info:
 	dma_free_coherent(&ha->pdev->dev,
 		((*rsp)->length + 1) * rsp_entry_size,
Signature · 1 line
2.47.3
author maintainer
#13 lore raw

PATCH 13/33 scsi: qla2xxx: Fix response queue over-consumption in __qla_consume_iocb()

In reply to Nilesh Javali

qla24xx_process_response_queue() advances ring_ptr past the head IOCB before dispatching, so by the time __qla_consume_iocb() runs, ring_ptr already points at the first continuation IOCB. The function however looped purex->entry_count times starting at ring_ptr. As entry_count includes the head, this consumed one entry too many: it stamped RESPONSE_PROCESSED on the next, unrelated IOCB and advanced the ring past it, silently dropping a legitimate firmware response. The head IOCB's signature was also never marked.

Mark the head processed and account for it, then consume only the entry_count - 1 continuation IOCBs, matching __qla_copy_purex_to_buffer().

Fixes fac2807946c1 ("scsi: qla2xxx: edif: Add extraction of auth_els from the wire")
---
 drivers/scsi/qla2xxx/qla_isr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/scsi/qla2xxx/qla_isr.c +11 -0
index 86e1e8428f2a..6375c18fe392 100644
@@ -259,6 +259,17 @@ void __qla_consume_iocb(struct scsi_qla_host *vha,
 	struct purex_entry_24xx *purex = *pkt;
 
 	entry_count_remaining = purex->entry_count;
+
+	/*
+	 * The caller already advanced ring_ptr past the head IOCB, so mark
+	 * the head processed and account for it here, then consume only the
+	 * continuation IOCBs that follow.
+	 */
+	((response_t *)purex)->signature = RESPONSE_PROCESSED;
+	/* flush signature */
+	wmb();
+	--entry_count_remaining;
+
 	while (entry_count_remaining > 0) {
 		new_pkt = rsp_q->ring_ptr;
 		*pkt = new_pkt;
Signature · 1 line
2.47.3
author maintainer
#14 lore raw

PATCH 14/33 scsi: qla2xxx: Fix soft lockup polling continuation IOCB signature

In reply to Nilesh Javali

qla27xx_copy_multiple_pkt() and qla27xx_copy_fpin_pkt() poll rsp_q->ring_ptr->signature for RESPONSE_PROCESSED (0xDEADDEAD) to decide whether the next continuation IOCB has arrived, spinning on cpu_relax() without advancing the ring or decrementing the entry count while it has not. response_t::signature lives at byte offset 60, but a continuation IOCB (sts_cont_entry_t / struct sts_cont_entry_ext) carries raw FC frame payload at that offset (data[56..59]). A received frame whose payload bytes happen to equal 0xDEADDEAD is therefore misread as "not yet arrived", and the loop spins forever in interrupt/DPC context, causing a CPU soft lockup.

The poll is also unnecessary: callers of qla27xx_copy_multiple_pkt() (PT_LS4_UNSOL and the NVMe purls path) already gate on qla_chk_cont_iocb_avail(), which guarantees all entry_count IOCBs are present before copying begins. The sibling helper __qla_copy_purex_to_buffer() already drops the signature poll and relies on the entry_type == STATUS_CONT_TYPE guard instead.

Remove the signature busy-wait from both helpers, keeping the entry_type guard, and gate the FPIN path with qla_chk_cont_iocb_avail() so it defers and re-processes on the next interrupt once all continuation IOCBs have arrived, mirroring the ELS_AUTH_ELS and PT_LS4_UNSOL arms. With this the signature field is never read on a continuation IOCB, eliminating the payload-aliasing lockup.

Fixes 9f2475fe7406 ("scsi: qla2xxx: SAN congestion management implementation")
---
 drivers/scsi/qla2xxx/qla_isr.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)
drivers/scsi/qla2xxx/qla_isr.c +16 -17
index 6375c18fe392..d9019f2b059c 100644
@@ -1002,14 +1002,6 @@ qla27xx_copy_multiple_pkt(struct scsi_qla_host *vha, void **pkt,
 
 	do {
 		while ((total_bytes > 0) && (entry_count_remaining > 0)) {
-			if (rsp_q->ring_ptr->signature == RESPONSE_PROCESSED) {
-				ql_dbg(ql_dbg_async, vha, 0x5084,
-				       "Ran out of IOCBs, partial data 0x%x\n",
-				       buffer_copy_offset);
-				cpu_relax();
-				continue;
-			}
-
 			*pkt = rsp_q->ring_ptr;
 			data = ((sts_cont_entry_t *)*pkt)->data;
 			data_sz = qla_sts_cont_data_size(ha);
@@ -1299,14 +1291,6 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
 
 	do {
 		while ((total_bytes > 0) && (entry_count_remaining > 0)) {
-			if (rsp_q->ring_ptr->signature == RESPONSE_PROCESSED) {
-				ql_dbg(ql_dbg_async, vha, 0x5084,
-				       "Ran out of IOCBs, partial data 0x%x\n",
-				       buffer_copy_offset);
-				cpu_relax();
-				continue;
-			}
-
 			*pkt = rsp_q->ring_ptr;
 			data = ((sts_cont_entry_t *)*pkt)->data;
 			data_sz = qla_sts_cont_data_size(ha);
@@ -4272,9 +4256,24 @@ void qla24xx_process_response_queue(struct scsi_qla_host *vha,
 					       "SCM not active for this port\n");
 					break;
 				}
+				if (qla_chk_cont_iocb_avail(vha, rsp,
+				    (response_t *)pkt, rsp_in)) {
+					/*
+					 * ring_ptr and ring_index were
+					 * pre-incremented above. Reset them
+					 * back to current. Wait for next
+					 * interrupt with all IOCBs to arrive
+					 * and re-process.
+					 */
+					qla_rsp_ring_rewind_to(rsp,
+					    (response_t *)pkt, cur_ring_index);
+
+					ql_dbg(ql_dbg_init, vha, 0x5095,
+					    "Defer processing FPIN...\n");
+					return;
+				}
 				pure_item = qla27xx_copy_fpin_pkt(vha,
 							  (void **)&pkt, &rsp);
-				__update_rsp_in(is_shadow_hba, rsp, rsp_in);
 				if (!pure_item)
 					break;
 				qla24xx_queue_purex_item(vha, pure_item,
Signature · 1 line
2.47.3
author maintainer
#15 lore raw

PATCH 15/33 scsi: qla2xxx: Bound rsp_info_len to avoid OOB sense-data read

In reply to Nilesh Javali

In qla2x00_status_entry(), the FWI2 status path advances sense_data and shrinks par_sense_len by rsp_info_len:

	if (IS_FWI2_CAPABLE(ha)) {
		sense_data += rsp_info_len;
		par_sense_len -= rsp_info_len;
	}

rsp_info_len is a 32-bit value taken directly from the target's FCP response (sf.rsp_data_len), while par_sense_len is the IOCB data area size (28 bytes for 24xx, 60 bytes for 29xx). A hostile or buggy target reporting an rsp_info_len larger than par_sense_len makes the unsigned subtraction underflow to a huge value and advances sense_data out of bounds.

The underflowed par_sense_len then defeats the cap in qla2x00_handle_sense():

	if (sense_len > par_sense_len)
		sense_len = par_sense_len;
	memcpy(cp->sense_buffer, sense_data, sense_len);

so the memcpy reads up to SCSI_SENSE_BUFFERSIZE bytes from the out-of-bounds sense_data pointer, leaking adjacent response-ring/heap memory into the command's sense buffer.

Clamp rsp_info_len to par_sense_len before the subtraction so par_sense_len can never underflow and sense_data stays within the IOCB data area. The fix sits before the comp_status switch, covering both qla2x00_handle_sense() call sites.

Fixes 5544213be7b4 ("[SCSI] qla2xxx: Correct extended sense-data handling.")
---
 drivers/scsi/qla2xxx/qla_isr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/scsi/qla2xxx/qla_isr.c +12 -0
index d9019f2b059c..02f88a79964a 100644
@@ -3598,6 +3598,18 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 	if (scsi_status & SS_RESPONSE_INFO_LEN_VALID) {
 		/* Sense data lies beyond any FCP RESPONSE data. */
 		if (IS_FWI2_CAPABLE(ha)) {
+			/*
+			 * A hostile or buggy target may report an
+			 * rsp_info_len larger than the IOCB data area.
+			 * Clamp it so the par_sense_len subtraction cannot
+			 * underflow and walk sense_data out of bounds.
+			 */
+			if (rsp_info_len > par_sense_len) {
+				ql_log(ql_log_warn, fcport->vha, 0x3107,
+				       "Truncating bogus rsp_info_len 0x%x to 0x%x.\n",
+				       rsp_info_len, par_sense_len);
+				rsp_info_len = par_sense_len;
+			}
 			sense_data += rsp_info_len;
 			par_sense_len -= rsp_info_len;
 		}
Signature · 1 line
2.47.3
author maintainer
#16 lore raw

PATCH 16/33 scsi: qla2xxx: Avoid req_q_map double-read in qla2x00_error_entry()

In reply to Nilesh Javali

qla2x00_error_entry() reads ha->req_q_map[que] twice: once for the NULL check and again when assigning it to req. The map slot is cleared by qla25xx_free_req_que() (ha->req_q_map[que_id] = NULL under mq_lock) during queue teardown, while the response-queue interrupt that drives qla2x00_error_entry() is still registered (the IRQ is released later in qla25xx_free_rsp_que()). If the slot is set to NULL between the two reads, req becomes NULL and is dereferenced.

Read the slot once into req and NULL-check the local before use. mq_lock is a mutex and cannot be taken from interrupt context, so the single read plus local check is the appropriate fix for the reported NULL dereference.

Fixes a6fe35c052c4 ("[SCSI] qla2xxx: Avoid invalid request queue dereference for bad response packets.")
---
 drivers/scsi/qla2xxx/qla_isr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_isr.c +3 -1
index 02f88a79964a..ddcfccfaef9c 100644
@@ -3928,10 +3928,12 @@ qla2x00_error_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, sts_entry_t *pkt)
 	    "iocb type %xh with error status %xh, handle %xh, rspq id %d\n",
 	    pkt->entry_type, pkt->entry_status, pkt->handle, rsp->id);
 
-	if (que >= ha->max_req_queues || !ha->req_q_map[que])
+	if (que >= ha->max_req_queues)
 		goto fatal;
 
 	req = ha->req_q_map[que];
+	if (!req)
+		goto fatal;
 
 	if (pkt->entry_status & RF_BUSY)
 		res = DID_BUS_BUSY << 16;
Signature · 1 line
2.47.3
author maintainer
#17 lore raw

PATCH 17/33 scsi: qla2xxx: Quiesce response IRQ before freeing request queue

In reply to Nilesh Javali

qla2xxx_delete_qpair() deletes the request queue before the response queue. qla25xx_delete_req_que() frees the request queue memory (kfree(req) in qla25xx_free_req_que()), but the response-queue MSI-X is only released later, in qla25xx_free_rsp_que(). In that window the response interrupt can still fire, qla2xxx_msix_rsp_q() queues qpair->q_work, and qla_do_work() -> qla24xx_process_response_queue() dereferences the now-freed rsp->req (LOGINOUT/CT/ELS entries and the status path), a use-after-free.

The cancel_work_sync() added for the qpair teardown lives in the response free path, which runs after the request queue is already freed, so it does not protect rsp->req.

Release the response-queue interrupt and flush qpair->q_work before deleting the request queue, so no late completion can reach the freed request queue. Clearing have_irq makes the subsequent qla25xx_free_rsp_que() skip its free_irq(), and the firmware queue-delete order (request then response) is preserved; the request-delete mailbox completes on the default vector and is unaffected by dropping the qpair response interrupt early.

Fixes d74595278f4a ("scsi: qla2xxx: Add multiple queue pair functionality.")
---
 drivers/scsi/qla2xxx/qla_init.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/scsi/qla2xxx/qla_init.c +17 -0
index fed6dbc3b6ae..e6b499245794 100644
@@ -10695,11 +10695,28 @@ int qla2xxx_delete_qpair(struct scsi_qla_host *vha, struct qla_qpair *qpair)
 {
 	int ret = QLA_FUNCTION_FAILED;
 	struct qla_hw_data *ha = qpair->hw;
+	struct rsp_que *rsp = qpair->rsp;
 
 	qpair->delete_in_progress = 1;
 
 	qla_free_buf_pool(qpair);
 
+	/*
+	 * The response-queue interrupt schedules qla_do_work(), which
+	 * dereferences qpair->rsp->req.  Release the interrupt and flush
+	 * any pending work before the request queue is freed below so a
+	 * late completion cannot touch the freed request queue.  The
+	 * firmware queue-delete order (request then response) is kept.
+	 */
+	if (rsp && rsp->msix && rsp->msix->have_irq) {
+		free_irq(rsp->msix->vector, rsp->msix->handle);
+		rsp->msix->have_irq = 0;
+		rsp->msix->in_use = 0;
+		rsp->msix->handle = NULL;
+	}
+	if (rsp && ha->wq)
+		cancel_work_sync(&qpair->q_work);
+
 	ret = qla25xx_delete_req_que(vha, qpair->req);
 	if (ret != QLA_SUCCESS)
 		goto fail;
Signature · 1 line
2.47.3
author maintainer
#18 lore raw

PATCH 18/33 scsi: qla2xxx: Reject non-SCSI SRB on status IOCB fast path

In reply to Nilesh Javali

qla2x00_status_entry() filters out non-TYPE_SRB entries and the SRB_NVME_CMD, SRB_BIDI_CMD and SRB_TM_CMD types, then falls through to a SCSI fast path that assumes the command is an SRB_SCSI_CMD. The first thing on that path, qla_chk_edif_rx_sa_delete_pending(), and the subsequent handling both evaluate GET_CMD_SP(sp), i.e. sp->u.scmd.cmd.

The srb u union overlays the SCSI command pointer with other command layouts (bsg_job, iocb_cmd). If firmware delivers an unexpected STATUS_TYPE IOCB for a non-SCSI handle, sp->u.scmd.cmd can read as a non-NULL garbage pointer, bypassing the NULL checks in qla_chk_edif_rx_sa_delete_pending() and at the cp == NULL test, and leading to a wild pointer dereference.

Reject any SRB whose type is not SRB_SCSI_CMD before entering the fast path. The outstanding_cmds slot is left untouched so a genuinely non-SCSI command still completes through its proper handler.

Fixes dd30706e73b7 ("scsi: qla2xxx: edif: Add key update")
---
 drivers/scsi/qla2xxx/qla_dbg.c | 2 +-
 drivers/scsi/qla2xxx/qla_isr.c | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_dbg.c +1 -1
index 4f756468ea64..196cfa8f8623 100644
@@ -16,7 +16,7 @@
  * |                              |                    | 0x2127-0x2128  |
  * | Queue Command and IO tracing |       0x3074       | 0x300b         |
  * |                              |                    | 0x3027-0x3028  |
- * |                              |                    | 0x303d-0x3041  |
+ * |                              |                    | 0x303e-0x3041  |
  * |                              |                    | 0x302e,0x3033  |
  * |                              |                    | 0x3036,0x3038  |
  * |                              |                    | 0x303a		|
drivers/scsi/qla2xxx/qla_isr.c +8 -0
index ddcfccfaef9c..63b70d4abbf0 100644
@@ -3541,6 +3541,14 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 		return;
 	}
 
+	/* Everything below is the SCSI fast path; reject other SRB types. */
+	if (sp->type != SRB_SCSI_CMD) {
+		ql_dbg(ql_dbg_io, vha, 0x303d,
+		    "Unexpected SRB type %x for status IOCB, sp %p.\n",
+		    sp->type, sp);
+		return;
+	}
+
 	/* Fast path completion. */
 	qla_chk_edif_rx_sa_delete_pending(vha, sp, pkt);
 	sp->qpair->cmd_completion_cnt++;
Signature · 1 line
2.47.3
author maintainer
#19 lore raw

PATCH 19/33 scsi: qla2xxx: Clamp max_npiv_vports to VP_CTRL bitmap capacity

In reply to Nilesh Javali

ha->max_npiv_vports is taken from firmware (mcp->mb[11]) and only constrained so that (max_npiv_vports + 1) is a multiple of MIN_MULTI_ID_FABRIC, which permits values of 63, 127, 191 and 255. NPIV vports are then allocated up to that count.

VP enable uses the VP_CONFIG IOCB, which addresses a vport through a plain vp_index byte, so a vp_index beyond 128 is enabled without issue. VP disable, however, uses the VP_CTRL IOCB, which selects target vports through the fixed 128-bit vp_idx_map bitmap. qla24xx_control_vp() rejects a vp_index past that bitmap and the IOCB builder cannot set a bit beyond 127, yet qla24xx_vport_delete() frees the local state regardless. A vport with vp_index > 128 can therefore be created and enabled but never disabled, leaving it permanently active in firmware: a resource leak.

Cap ha->max_npiv_vports at init to the vp_idx_map capacity so such vports are never created. This collapses 191/255 to 127 (still modulo-valid) and leaves the real-world 63/127 cases unaffected.

Fixes 4d0ea24769c8 ("[SCSI] qla2xxx: Retrieve max-NPIV support capabilities from FW.")
---
 drivers/scsi/qla2xxx/qla_fw.h   |  4 ++++
 drivers/scsi/qla2xxx/qla_init.c | 13 +++++++++++++
 drivers/scsi/qla2xxx/qla_mid.c  |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_fw.h +4 -0
index b29abcc7f74f..98bc4a57b59b 100644
@@ -1442,6 +1442,10 @@ struct vp_ctrl_entry_24xx {
 	uint8_t reserved_5[24];
 };
 
+/* vp_idx_map is a 128-bit (16-byte) bitmap selecting target VPs. */
+#define VP_CTRL_IDX_MAP_BITS \
+	(sizeof_field(struct vp_ctrl_entry_24xx, vp_idx_map) * 8)
+
 /*
  * Modify Virtual Port Configuration IOCB
  */
drivers/scsi/qla2xxx/qla_init.c +13 -0
index e6b499245794..5788c7e53d8f 100644
@@ -4412,6 +4412,19 @@ qla2x00_setup_chip(scsi_qla_host_t *vha)
 					    MIN_MULTI_ID_FABRIC))
 						ha->max_npiv_vports =
 						    MIN_MULTI_ID_FABRIC - 1;
+
+					/*
+					 * The VP_CTRL IOCB selects target VPs
+					 * through the fixed vp_idx_map bitmap,
+					 * so a vp_index beyond it can be enabled
+					 * via VP_CONFIG but never disabled via
+					 * VP_CTRL, leaking the VP.  Cap the count
+					 * to the bitmap capacity.
+					 */
+					if (ha->max_npiv_vports >=
+					    VP_CTRL_IDX_MAP_BITS)
+						ha->max_npiv_vports =
+						    VP_CTRL_IDX_MAP_BITS - 1;
 				}
 				qlt_config_nvram_with_fw_version(vha);
 				qla2x00_get_resource_cnts(vha);
drivers/scsi/qla2xxx/qla_mid.c +1 -1
index 33bfc61d8165..4ad23d206add 100644
@@ -996,7 +996,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
 	 * (16-byte) vp_idx_map bitmap, so vp_index must fit within it even
 	 * if firmware advertises more NPIV vports.
 	 */
-	if (vp_index > sizeof_field(struct vp_ctrl_entry_24xx, vp_idx_map) * 8)
+	if (vp_index > VP_CTRL_IDX_MAP_BITS)
 		return QLA_PARAMETER_ERROR;
 
 	/* ref: INIT */
Signature · 1 line
2.47.3
author maintainer
#20 lore raw

PATCH 20/33 scsi: qla2xxx: Avoid double completion in async IOCB timeout

In reply to Nilesh Javali

qla2x00_async_iocb_timeout() tries to abort a timed-out async IOCB. When qla24xx_async_abort_cmd() fails, both the SRB_LOGIN_CMD path and the SRB_CTRL_VP/default path scan outstanding_cmds[] for the SRB and then call sp->done(sp, QLA_FUNCTION_TIMEOUT) unconditionally, without checking whether the SRB was actually found and removed.

If the response ISR completes the same handle first, it removes the SRB under qp_lock_ptr and runs sp->done() -> complete(sp->comp). The submitter qla24xx_control_vp() wakes from wait_for_completion(), clears sp->comp, drops its reference and returns, reclaiming the on-stack completion. The timer reference keeps the SRB alive across the timeout handler, but not the submitter's stack. The timeout then issues a second sp->done() -> qla_ctrlvp_sp_done(), which evaluates "if (sp->comp) complete(sp->comp)"; with the pointer loaded before the submitter's NULL store, complete() writes into the freed stack frame, a use-after-free.

Track whether this path removed the SRB from outstanding_cmds and only call sp->done() when it did, so the command is completed exactly once by whichever path owns it. This mirrors the sp_found guard already used in qla24xx_abort_iocb_timeout().

Fixes f6145e86d21f ("scsi: qla2xxx: Fix race between switch cmd completion and timeout")
---
 drivers/scsi/qla2xxx/qla_init.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
drivers/scsi/qla2xxx/qla_init.c +21 -3
index 5788c7e53d8f..36de0a0bbcc1 100644
@@ -228,7 +228,7 @@ qla2x00_async_iocb_timeout(void *data)
 	srb_t *sp = data;
 	fc_port_t *fcport = sp->fcport;
 	struct srb_iocb *lio = &sp->u.iocb_cmd;
-	int rc, h;
+	int rc, h, found;
 	unsigned long flags;
 
 	if (fcport) {
@@ -251,6 +251,7 @@ qla2x00_async_iocb_timeout(void *data)
 			lio->u.logio.data[1] =
 				lio->u.logio.flags & SRB_LOGIN_RETRIED ?
 				QLA_LOGIO_LOGIN_RETRIED : 0;
+			found = 0;
 			spin_lock_irqsave(sp->qpair->qp_lock_ptr, flags);
 			for (h = 1; h < sp->qpair->req->num_outstanding_cmds;
 			    h++) {
@@ -258,11 +259,19 @@ qla2x00_async_iocb_timeout(void *data)
 				    sp) {
 					sp->qpair->req->outstanding_cmds[h] =
 					    NULL;
+					found = 1;
 					break;
 				}
 			}
 			spin_unlock_irqrestore(sp->qpair->qp_lock_ptr, flags);
-			sp->done(sp, QLA_FUNCTION_TIMEOUT);
+			/*
+			 * Only complete the command if this path removed it
+			 * from outstanding_cmds.  Otherwise the ISR already
+			 * completed it and a second sp->done() would race the
+			 * submitter's freeing of the on-stack completion.
+			 */
+			if (found)
+				sp->done(sp, QLA_FUNCTION_TIMEOUT);
 		}
 		break;
 	case SRB_LOGOUT_CMD:
@@ -275,6 +284,7 @@ qla2x00_async_iocb_timeout(void *data)
 	default:
 		rc = qla24xx_async_abort_cmd(sp, false);
 		if (rc) {
+			found = 0;
 			spin_lock_irqsave(sp->qpair->qp_lock_ptr, flags);
 			for (h = 1; h < sp->qpair->req->num_outstanding_cmds;
 			    h++) {
@@ -282,11 +292,19 @@ qla2x00_async_iocb_timeout(void *data)
 				    sp) {
 					sp->qpair->req->outstanding_cmds[h] =
 					    NULL;
+					found = 1;
 					break;
 				}
 			}
 			spin_unlock_irqrestore(sp->qpair->qp_lock_ptr, flags);
-			sp->done(sp, QLA_FUNCTION_TIMEOUT);
+			/*
+			 * Only complete the command if this path removed it
+			 * from outstanding_cmds.  Otherwise the ISR already
+			 * completed it and a second sp->done() would race the
+			 * submitter's freeing of the on-stack completion.
+			 */
+			if (found)
+				sp->done(sp, QLA_FUNCTION_TIMEOUT);
 		}
 		break;
 	}
Signature · 1 line
2.47.3
author maintainer
#21 lore raw

PATCH 21/33 scsi: qla2xxx: Skip vport under deletion in report ID acquisition

In reply to Nilesh Javali

qla24xx_report_id_acquisition() format-1 handling walks ha->vp_list under vport_slock, takes a vref_count on the matching vport and calls qla_update_host_map() to register its port id.

A vport teardown via qla24xx_vport_delete() sets VPORT_DELETE, then qla24xx_disable_vp() removes the vport from the host_map btree and zeroes vha->d_id (RESET_AL_PA). The vport is only unlinked from vp_list later, in qla24xx_deallocate_vp_id(), which clears vp_map[idx] (RESET_VP_IDX) but does not touch host_map. In the window in between, report ID acquisition can still find the vport on vp_list and call qla_update_host_map(); with d_id already zeroed it takes the btree_insert32() path and re-inserts the dying vport into host_map. Nothing cleans that entry afterwards, so once scsi_host_put() frees the vha a later host_map lookup dereferences freed memory.

Skip a vport that has VPORT_DELETE set before taking the reference, so it is neither re-registered nor scheduled for DPC re-registration. This mirrors the existing guard in qla2x00_alert_all_vps().

Fixes 41dc529a4602 ("qla2xxx: Improve RSCN handling in driver")
---
 drivers/scsi/qla2xxx/qla_mbx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/qla2xxx/qla_mbx.c +2 -0
index ab5648eb5f20..affcd87893cd 100644
@@ -4268,6 +4268,8 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
 			spin_lock_irqsave(&ha->vport_slock, flags);
 			list_for_each_entry(vp, &ha->vp_list, list) {
 				if (vp_idx == vp->vp_idx) {
+					if (test_bit(VPORT_DELETE, &vp->dpc_flags))
+						break;
 					found = 1;
 					atomic_inc(&vp->vref_count);
 					break;
Signature · 1 line
2.47.3
author maintainer
#22 lore raw

PATCH 22/33 scsi: qla2xxx: Drop vport reference under lock in report ID acquisition

In reply to Nilesh Javali

qla24xx_report_id_acquisition() format-1 handling takes the vport reference under vport_slock but drops it outside the lock, after setting vp->vp_flags and vp->dpc_flags:

	set_bit(VP_IDX_ACQUIRED, &vp->vp_flags);
	set_bit(REGISTER_FC4_NEEDED, &vp->dpc_flags);
	set_bit(REGISTER_FDMI_NEEDED, &vp->dpc_flags);

	atomic_dec(&vp->vref_count);

Neither set_bit() nor atomic_dec() imply a memory barrier, so on a weakly ordered architecture the decrement can become visible before the flag stores. qla24xx_deallocate_vp_id() polls vref_count under vport_slock and unlinks the vport once it reads zero, after which qla24xx_vport_delete() frees it via scsi_host_put(). The poller could therefore observe vref_count == 0 early and tear the vport down while the pending vp_flags/ dpc_flags stores land on freed memory.

Drop the reference under vport_slock, as is done for the matching increment and by every other vref_count user. The unlock release pairs with the deallocate poller's lock acquire so the flag stores are ordered before vref_count == 0 can be observed.

Fixes 87c20ed7521c ("scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()")
---
 drivers/scsi/qla2xxx/qla_mbx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/qla2xxx/qla_mbx.c +2 -0
index affcd87893cd..e88c3a989a51 100644
@@ -4290,7 +4290,9 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
 			set_bit(REGISTER_FC4_NEEDED, &vp->dpc_flags);
 			set_bit(REGISTER_FDMI_NEEDED, &vp->dpc_flags);
 
+			spin_lock_irqsave(&ha->vport_slock, flags);
 			atomic_dec(&vp->vref_count);
+			spin_unlock_irqrestore(&ha->vport_slock, flags);
 		}
 		set_bit(VP_DPC_NEEDED, &vha->dpc_flags);
 		qla2xxx_wake_dpc(vha);
Signature · 1 line
2.47.3
author maintainer
#23 lore raw

PATCH 23/33 scsi: qla2xxx: Hold vport_slock for host map update in report ID acquisition

In reply to Nilesh Javali

qla24xx_report_id_acquisition() format-1 handling drops vport_slock after taking the vport reference and then calls qla_update_host_map() without the lock. That reaches qla_update_vp_map(), which mutates the ha->host_map btree via btree_insert32()/btree_update32()/btree_remove32() and is documented to require vport_slock to be held by the caller. Running it unlocked can race concurrent host_map updates and corrupt the btree.

The format-2 path in the same function already wraps its host_map update (SET_AL_PA) in vport_slock; the format-1 path is the lone outlier.

Hold vport_slock across the format-1 qla_update_host_map() call to honor the documented locking contract. The vref_count taken in the loop keeps the vport valid, so this only adds the missing host_map serialization.

Fixes 430eef03a763 ("scsi: qla2xxx: Relocate/rename vp map")
---
 drivers/scsi/qla2xxx/qla_mbx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/qla2xxx/qla_mbx.c +2 -0
index e88c3a989a51..39544deab576 100644
@@ -4280,7 +4280,9 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
 			if (!found)
 				return;
 
+			spin_lock_irqsave(&ha->vport_slock, flags);
 			qla_update_host_map(vp, id);
+			spin_unlock_irqrestore(&ha->vport_slock, flags);
 
 			/*
 			 * Cannot configure here as we are still sitting on the
Signature · 1 line
2.47.3
author maintainer
#24 lore raw

PATCH 24/33 scsi: qla2xxx: Fix NVMe abort reference leak on repeated abort

In reply to Nilesh Javali

qla_nvme_ls_abort() and qla_nvme_fcp_abort() take a command reference with kref_get_unless_zero() and then call schedule_work() on priv->abort_work, ignoring its return value. qla_nvme_abort_work() runs once and drops exactly one reference via kref_put(&sp->cmd_kref, sp->put_fn).

Since the per-abort INIT_WORK() was moved to submission time, schedule_work() now returns false when the work is already pending, for example on a concurrent transport teardown and timeout-driven abort of the same command. In that case the reference taken for the second abort is never released because the work still executes only once, leaking a reference. The command is then never returned to the NVMe-FC transport, which can hang the port.

Drop the reference when schedule_work() returns false, so each kref_get_unless_zero() is balanced regardless of whether the work was newly queued. The held reference keeps priv->sp valid for the put.

Fixes 70cbb6fdd31b ("scsi: qla2xxx: Initialize NVMe abort_work once at submission")
---
 drivers/scsi/qla2xxx/qla_nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/scsi/qla2xxx/qla_nvme.c +4 -2
index 8dc6df6c2e1c..fc8a344ec7d8 100644
@@ -466,7 +466,8 @@ static void qla_nvme_ls_abort(struct nvme_fc_local_port *lport,
 	}
 	spin_unlock_irqrestore(&priv->cmd_lock, flags);
 
-	schedule_work(&priv->abort_work);
+	if (!schedule_work(&priv->abort_work))
+		kref_put(&priv->sp->cmd_kref, priv->sp->put_fn);
 }
 
 static int qla_nvme_ls_req(struct nvme_fc_local_port *lport,
@@ -548,7 +549,8 @@ static void qla_nvme_fcp_abort(struct nvme_fc_local_port *lport,
 	}
 	spin_unlock_irqrestore(&priv->cmd_lock, flags);
 
-	schedule_work(&priv->abort_work);
+	if (!schedule_work(&priv->abort_work))
+		kref_put(&priv->sp->cmd_kref, priv->sp->put_fn);
 }
 
 static inline int qla2x00_start_nvme_mq(srb_t *sp)
Signature · 1 line
2.47.3
author maintainer
#25 lore raw

PATCH 25/33 scsi: qla2xxx: Skip NVMe LS reject IOCB when FW not started

In reply to Nilesh Javali

qla_nvme_xmt_ls_rsp() bails out to the out: label when firmware is not started (!ha->flags.fw_started), but the out: path unconditionally calls qla_nvme_ls_reject_iocb(), which ends in qla2x00_start_iocbs() and an unconditional doorbell write to the request queue in-pointer register. This rings the firmware doorbell and queues an IOCB that stopped or resetting firmware cannot consume, and touches MMIO during the reset/EEH window where fw_started is also clear.

Only emit the LS reject IOCB (and ring the doorbell) when fw_started is set; otherwise just clean up and return. The post-allocation failure cases (SRB alloc / qla2x00_start_sp() failure) run with firmware started and still send the reject. Apply the same guard to the reject emission in qla2xxx_process_purls_pkt().

Fixes 875386b98857 ("scsi: qla2xxx: Add Unsolicited LS Request and Response Support for NVMe")
---
 drivers/scsi/qla2xxx/qla_nvme.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/scsi/qla2xxx/qla_nvme.c +13 -6
index fc8a344ec7d8..28a04e0ff660 100644
@@ -441,9 +441,11 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
 	a.vp_idx = vha->vp_idx;
 	a.nport_handle = uctx->nport_handle;
 	a.xchg_address = uctx->exchange_address;
-	spin_lock_irqsave(ha->base_qpair->qp_lock_ptr, flags);
-	qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
-	spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);
+	if (ha->flags.fw_started) {
+		spin_lock_irqsave(ha->base_qpair->qp_lock_ptr, flags);
+		qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
+		spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);
+	}
 	kfree(uctx);
 	return rval;
 }
@@ -1321,9 +1323,14 @@ qla2xxx_process_purls_pkt(struct scsi_qla_host *vha, struct purex_item *item)
 		a.vp_idx = vha->vp_idx;
 		a.nport_handle = uctx->nport_handle;
 		a.xchg_address = uctx->exchange_address;
-		spin_lock_irqsave(vha->hw->base_qpair->qp_lock_ptr, flags);
-		qla_nvme_ls_reject_iocb(vha, vha->hw->base_qpair, &a, true);
-		spin_unlock_irqrestore(vha->hw->base_qpair->qp_lock_ptr, flags);
+		if (vha->hw->flags.fw_started) {
+			spin_lock_irqsave(vha->hw->base_qpair->qp_lock_ptr,
+					  flags);
+			qla_nvme_ls_reject_iocb(vha, vha->hw->base_qpair, &a,
+						true);
+			spin_unlock_irqrestore(vha->hw->base_qpair->qp_lock_ptr,
+					       flags);
+		}
 		list_del(&uctx->elem);
 		kfree(uctx);
 	}
Signature · 1 line
2.47.3
author maintainer
#26 lore raw

PATCH 26/33 scsi: qla2xxx: Unlink NVMe unsol ctx before freeing on LS reject error

In reply to Nilesh Javali

qla_nvme_xmt_ls_rsp() obtains uctx, which was linked into fcport->unsol_ctx_head by qla2xxx_process_purls_iocb() and is still linked when the NVMe transport calls back to transmit the LS response. On the error (out:) path the function frees uctx with kfree() but never removes it from the list. This leaves a freed node in fcport->unsol_ctx_head: the next list_add_tail() for that fcport writes through the freed node, and a subsequent list_del() can corrupt the list or panic.

Unlink uctx with list_del() before kfree() on the error path, matching the other free sites in qla_nvme_release_lsrsp_cmd_kref() and qla2xxx_process_purls_pkt(). qla2x00_rel_sp() in the failure path only returns the SRB to its pool and does not invoke sp->put_fn, so the out:
path is the sole free and uctx is always still linked there.

Fixes 875386b98857 ("scsi: qla2xxx: Add Unsolicited LS Request and Response Support for NVMe")
---
 drivers/scsi/qla2xxx/qla_nvme.c | 1 +
 1 file changed, 1 insertion(+)
drivers/scsi/qla2xxx/qla_nvme.c +1 -0
index 28a04e0ff660..36b742f73abf 100644
@@ -446,6 +446,7 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
 		qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
 		spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);
 	}
+	list_del(&uctx->elem);
 	kfree(uctx);
 	return rval;
 }
Signature · 1 line
2.47.3
author maintainer
#27 lore raw

PATCH 27/33 scsi: qla2xxx: Serialize NVMe unsol ctx list with a per-fcport lock

In reply to Nilesh Javali

The fcport->unsol_ctx_head list is modified from several contexts without a common lock. Entries are added in qla2xxx_process_purls_iocb() from the response queue ISR (under the qpair qp_lock), while they are removed from qla2xxx_process_purls_pkt() (DPC/purex worker), qla_nvme_xmt_ls_rsp() (NVMe-FC transport callback) and qla_nvme_release_lsrsp_cmd_kref() (SRB completion). The qpair qp_lock cannot serialize this per-fcport list since multiqueue adapters add entries through different qpairs, so a concurrent add and delete (or two concurrent deletes) can corrupt the list pointers.

Introduce a dedicated per-fcport spinlock, unsol_ctx_lock, initialized in qla2x00_alloc_fcport(), and take it around every list_add_tail()/list_del() on unsol_ctx_head. The add nests under the existing qp_lock; no delete path takes qp_lock, so the lock order is consistent and deadlock free.

Fixes 875386b98857 ("scsi: qla2xxx: Add Unsolicited LS Request and Response Support for NVMe")
---
 drivers/scsi/qla2xxx/qla_def.h  | 2 ++
 drivers/scsi/qla2xxx/qla_init.c | 1 +
 drivers/scsi/qla2xxx/qla_nvme.c | 9 +++++++++
 3 files changed, 12 insertions(+)
drivers/scsi/qla2xxx/qla_def.h +2 -0
index 2684d785ecbf..97e2a1a9ce3f 100644
@@ -2645,6 +2645,8 @@ typedef struct fc_port {
 	struct list_head list;
 	struct scsi_qla_host *vha;
 	struct list_head unsol_ctx_head;
+	/* Serializes unsol_ctx_head against ISR, DPC and NVMe transport. */
+	spinlock_t unsol_ctx_lock;
 
 	unsigned int conf_compl_supported:1;
 	unsigned int deleted:2;
drivers/scsi/qla2xxx/qla_init.c +1 -0
index 36de0a0bbcc1..2b9a9c672ec6 100644
@@ -5710,6 +5710,7 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags)
 	INIT_LIST_HEAD(&fcport->gnl_entry);
 	INIT_LIST_HEAD(&fcport->list);
 	INIT_LIST_HEAD(&fcport->unsol_ctx_head);
+	spin_lock_init(&fcport->unsol_ctx_lock);
 
 	INIT_LIST_HEAD(&fcport->sess_cmd_list);
 	spin_lock_init(&fcport->sess_cmd_lock);
drivers/scsi/qla2xxx/qla_nvme.c +9 -0
index 36b742f73abf..beccece1e7d9 100644
@@ -257,7 +257,9 @@ static void qla_nvme_release_lsrsp_cmd_kref(struct kref *kref)
 
 	fd_rsp = uctx->fd_rsp;
 
+	spin_lock_irqsave(&uctx->fcport->unsol_ctx_lock, flags);
 	list_del(&uctx->elem);
+	spin_unlock_irqrestore(&uctx->fcport->unsol_ctx_lock, flags);
 
 	fd_rsp->done(fd_rsp);
 	kfree(uctx);
@@ -446,7 +448,9 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
 		qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
 		spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);
 	}
+	spin_lock_irqsave(&uctx->fcport->unsol_ctx_lock, flags);
 	list_del(&uctx->elem);
+	spin_unlock_irqrestore(&uctx->fcport->unsol_ctx_lock, flags);
 	kfree(uctx);
 	return rval;
 }
@@ -1332,7 +1336,9 @@ qla2xxx_process_purls_pkt(struct scsi_qla_host *vha, struct purex_item *item)
 			spin_unlock_irqrestore(vha->hw->base_qpair->qp_lock_ptr,
 					       flags);
 		}
+		spin_lock_irqsave(&uctx->fcport->unsol_ctx_lock, flags);
 		list_del(&uctx->elem);
+		spin_unlock_irqrestore(&uctx->fcport->unsol_ctx_lock, flags);
 		kfree(uctx);
 	}
 }
@@ -1374,6 +1380,7 @@ void qla2xxx_process_purls_iocb(void **pkt, struct rsp_que **rsp)
 	struct purex_item *item;
 	port_id_t d_id = {0};
 	port_id_t id = {0};
+	unsigned long flags;
 	u8 *opcode;
 	bool xmt_reject = false;
 
@@ -1439,7 +1446,9 @@ void qla2xxx_process_purls_iocb(void **pkt, struct rsp_que **rsp)
 	uctx->ox_id = p->ox_id;
 	qla_rport->uctx = uctx;
 	INIT_LIST_HEAD(&uctx->elem);
+	spin_lock_irqsave(&fcport->unsol_ctx_lock, flags);
 	list_add_tail(&uctx->elem, &fcport->unsol_ctx_head);
+	spin_unlock_irqrestore(&fcport->unsol_ctx_lock, flags);
 	item->purls_context = (void *)uctx;
 
 	ql_dbg(ql_dbg_unsol, vha, 0x2121,
Signature · 1 line
2.47.3
author maintainer
#28 lore raw

PATCH 28/33 scsi: qla2xxx: Use coherent DMA buffer for D_Port diagnostics

In reply to Nilesh Javali

qla26xx_dport_diagnostics() streaming-maps the caller's result buffer with dma_map_single(). The bsg path passes &dd->buf from the __packed struct qla_dport_diag, where buf lands at a 2-byte offset and shares cachelines with the surrounding options/unused fields. Mapping such a misaligned sub-buffer violates the DMA API requirement that streaming buffers be cacheline aligned and not share a cacheline with other data, and can corrupt data on non-DMA-coherent architectures.

Allocate a dedicated DMA-coherent buffer inside qla26xx_dport_diagnostics() for the mailbox command and copy the result back into the caller's buffer. This removes the streaming map of the misaligned sub-buffer entirely; the caller's buffer is now only a plain CPU buffer, so its packing no longer matters.

Fixes ec89146215d1 ("qla2xxx: Add bsg interface to support D_Port Diagnostics.")
---
 drivers/scsi/qla2xxx/qla_mbx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/scsi/qla2xxx/qla_mbx.c +7 -8
index 39544deab576..59ec5605930b 100644
@@ -6579,6 +6579,7 @@ qla26xx_dport_diagnostics(scsi_qla_host_t *vha,
 	mbx_cmd_t mc;
 	mbx_cmd_t *mcp = &mc;
 	dma_addr_t dd_dma;
+	void *dd;
 
 	if (!IS_QLA83XX(vha->hw) && !IS_QLA27XX(vha->hw) &&
 	    !IS_QLA28XX(vha->hw) && !IS_QLA29XX(vha->hw))
@@ -6587,15 +6588,12 @@ qla26xx_dport_diagnostics(scsi_qla_host_t *vha,
 	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x119f,
 	    "Entered %s.\n", __func__);
 
-	dd_dma = dma_map_single(&vha->hw->pdev->dev,
-	    dd_buf, size, DMA_FROM_DEVICE);
-	if (dma_mapping_error(&vha->hw->pdev->dev, dd_dma)) {
-		ql_log(ql_log_warn, vha, 0x1194, "Failed to map dma buffer.\n");
+	dd = dma_alloc_coherent(&vha->hw->pdev->dev, size, &dd_dma, GFP_KERNEL);
+	if (!dd) {
+		ql_log(ql_log_warn, vha, 0x1194, "Failed to allocate dma buffer.\n");
 		return QLA_MEMORY_ALLOC_FAILED;
 	}
 
-	memset(dd_buf, 0, size);
-
 	mcp->mb[0] = MBC_DPORT_DIAGNOSTICS;
 	mcp->mb[1] = options;
 	mcp->mb[2] = MSW(LSD(dd_dma));
@@ -6617,8 +6615,9 @@ qla26xx_dport_diagnostics(scsi_qla_host_t *vha,
 		    "Done %s.\n", __func__);
 	}
 
-	dma_unmap_single(&vha->hw->pdev->dev, dd_dma,
-	    size, DMA_FROM_DEVICE);
+	memcpy(dd_buf, dd, size);
+
+	dma_free_coherent(&vha->hw->pdev->dev, size, dd, dd_dma);
 
 	return rval;
 }
Signature · 1 line
2.47.3
author maintainer
#29 lore raw

PATCH 29/33 scsi: qla2xxx: Zero-init bsg stack buffers to avoid info leak

In reply to Nilesh Javali

Several bsg handlers stage their request/reply in an uninitialized 256-byte on-stack buffer (uint8_t bsg[DMA_POOL_SIZE]) and fill it via sg_copy_to_buffer(), which only copies as many bytes as the user-supplied request payload. When the request is shorter than the structure, the remainder of the buffer is left holding stale stack data.

qla2x00_read_fru_status() and qla2x00_read_i2c() then copy the full structure back to the reply payload with sg_copy_from_buffer(), leaking the uninitialized stack bytes to user space. The write/update paths do not copy the buffer back, but can feed uninitialized fields to the device.

Zero the stack buffer at declaration in all five handlers, mirroring the heap kzalloc() approach, so short requests can no longer expose stale memory.

Fixes 697a4bc69159 ("[SCSI] qla2xxx: Provide method for updating I2C attached VPD.")
Fixes 9ebb5d9c69f1 ("[SCSI] qla2xxx: Add I2C BSG interface.")
---
 drivers/scsi/qla2xxx/qla_bsg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/scsi/qla2xxx/qla_bsg.c +5 -5
index f9b693af8db1..49f66f37b1a7 100644
@@ -1973,7 +1973,7 @@ qla2x00_update_fru_versions(struct bsg_job *bsg_job)
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
 	int rval = 0;
-	uint8_t bsg[DMA_POOL_SIZE];
+	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_image_version_list *list = (void *)bsg;
 	struct qla_image_version *image;
 	uint32_t count;
@@ -2033,7 +2033,7 @@ qla2x00_read_fru_status(struct bsg_job *bsg_job)
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
 	int rval = 0;
-	uint8_t bsg[DMA_POOL_SIZE];
+	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_status_reg *sr = (void *)bsg;
 	dma_addr_t sfp_dma;
 	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
@@ -2084,7 +2084,7 @@ qla2x00_write_fru_status(struct bsg_job *bsg_job)
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
 	int rval = 0;
-	uint8_t bsg[DMA_POOL_SIZE];
+	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_status_reg *sr = (void *)bsg;
 	dma_addr_t sfp_dma;
 	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
@@ -2131,7 +2131,7 @@ qla2x00_write_i2c(struct bsg_job *bsg_job)
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
 	int rval = 0;
-	uint8_t bsg[DMA_POOL_SIZE];
+	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_i2c_access *i2c = (void *)bsg;
 	dma_addr_t sfp_dma;
 	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
@@ -2177,7 +2177,7 @@ qla2x00_read_i2c(struct bsg_job *bsg_job)
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
 	int rval = 0;
-	uint8_t bsg[DMA_POOL_SIZE];
+	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_i2c_access *i2c = (void *)bsg;
 	dma_addr_t sfp_dma;
 	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
Signature · 1 line
2.47.3
author maintainer
#30 lore raw

PATCH 30/33 scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]

In reply to Nilesh Javali

The FC BSG transport allocates job->request via memdup_user() using the exact user-supplied request_len. For FC_BSG_HST_VENDOR, fc_bsg_host_dispatch() only guarantees request_len covers msgcode and vendor_id; it does not account for the vendor_cmd[] flexible array.

qla2xxx then reads the command selector vendor_cmd[0] and, in several sub-handlers, vendor_cmd[1]/[2] or structures overlaid on the vendor command area without verifying request_len. A caller holding CAP_SYS_RAWIO can submit a short request whose vendor_id matches the host, triggering out-of-bounds heap reads (KASAN-detectable, and able to mis-select a command or panic).

Add a central guard in qla2x00_process_vendor_specific() so the selector is always in bounds, restrict the early vendor_cmd[0] read in qla24xx_bsg_request() to sufficiently long vendor messages, and add request_len checks to the sub-handlers that read further:
qla24xx_proc_fcp_prio_cfg_cmd(), qla2x00_process_loopback(), qla84xx_reset(), qla84xx_updatefw(), qla2x00_read_optrom(), qla2x00_update_optrom(), qlafx00_mgmt_cmd() and qla28xx_validate_flash_image().

Fixes 01e0e15c8b3b ("scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly")
---
 drivers/scsi/qla2xxx/qla_bsg.c | 57 +++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 4 deletions(-)
drivers/scsi/qla2xxx/qla_bsg.c +53 -4
index 49f66f37b1a7..7fe7480fdc1b 100644
@@ -160,6 +160,12 @@ qla24xx_proc_fcp_prio_cfg_cmd(struct bsg_job *bsg_job)
 		goto exit_fcp_prio_cfg;
 	}
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t)) {
+		ret = -EINVAL;
+		goto exit_fcp_prio_cfg;
+	}
+
 	/* Get the sub command */
 	oper = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
 
@@ -758,6 +764,10 @@ qla2x00_process_loopback(struct bsg_job *bsg_job)
 		return -EIO;
 	}
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 3 * sizeof(uint32_t))
+		return -EINVAL;
+
 	memset(&elreq, 0, sizeof(elreq));
 
 	elreq.req_sg_cnt = dma_map_sg(&ha->pdev->dev,
@@ -990,6 +1000,10 @@ qla84xx_reset(struct bsg_job *bsg_job)
 		return -EINVAL;
 	}
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
+		return -EINVAL;
+
 	flag = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
 
 	rval = qla84xx_reset_chip(vha, flag == A84_ISSUE_RESET_DIAG_FW);
@@ -1034,6 +1048,10 @@ qla84xx_updatefw(struct bsg_job *bsg_job)
 		return -EINVAL;
 	}
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
+		return -EINVAL;
+
 	sg_cnt = dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
 		bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
 	if (!sg_cnt) {
@@ -1511,9 +1529,15 @@ qla2x00_read_optrom(struct bsg_job *bsg_job)
 	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
-	uint32_t start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
+	uint32_t start;
 	int rval = 0;
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
+		return -EINVAL;
+
+	start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
+
 	if (ha->flags.nic_core_reset_hdlr_active)
 		return -EBUSY;
 
@@ -1556,9 +1580,15 @@ qla2x00_update_optrom(struct bsg_job *bsg_job)
 	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
 	scsi_qla_host_t *vha = shost_priv(host);
 	struct qla_hw_data *ha = vha->hw;
-	uint32_t start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
+	uint32_t start;
 	int rval = 0;
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
+		return -EINVAL;
+
+	start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
+
 	mutex_lock(&ha->optrom_mutex);
 	rval = qla2x00_optrom_setup(bsg_job, vha, start, 1);
 	if (rval) {
@@ -2411,6 +2441,11 @@ qlafx00_mgmt_cmd(struct bsg_job *bsg_job)
 	struct fc_port *fcport;
 	char  *type = "FC_BSG_HST_FX_MGMT";
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + sizeof(uint32_t) +
+	    sizeof(struct qla_mt_iocb_rqst_fx00))
+		return -EINVAL;
+
 	/* Copy the IOCB specific information */
 	piocb_rqst = (struct qla_mt_iocb_rqst_fx00 *)
 	    &bsg_request->rqst_data.h_vendor.vendor_cmd[1];
@@ -3332,6 +3367,13 @@ qla2x00_process_vendor_specific(struct scsi_qla_host *vha, struct bsg_job *bsg_j
 {
 	struct fc_bsg_request *bsg_request = bsg_job->request;
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + sizeof(uint32_t)) {
+		ql_log(ql_log_warn, vha, 0x7000,
+		       "BSG request too small for vendor cmd.\n");
+		return -EINVAL;
+	}
+
 	ql_dbg(ql_dbg_edif, vha, 0x911b, "%s FC_BSG_HST_VENDOR cmd[0]=0x%x\n",
 	    __func__, bsg_request->rqst_data.h_vendor.vendor_cmd[0]);
 
@@ -3475,8 +3517,11 @@ qla24xx_bsg_request(struct bsg_job *bsg_job)
 	}
 
 	/* Disable port will bring down the chip, allow enable command */
-	if (bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_MANAGE_HOST_PORT ||
-	    bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_GET_HOST_STATS)
+	if (bsg_request->msgcode == FC_BSG_HST_VENDOR &&
+	    bsg_job->request_len >=
+		sizeof(struct fc_bsg_request) + sizeof(uint32_t) &&
+	    (bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_MANAGE_HOST_PORT ||
+	     bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_GET_HOST_STATS))
 		goto skip_chip_chk;
 
 	if (vha->hw->flags.port_isolated) {
@@ -3785,6 +3830,10 @@ static int qla28xx_validate_flash_image(struct bsg_job *bsg_job)
 	if (!IS_QLA28XX(ha) || vha->vp_idx != 0)
 		return -EPERM;
 
+	if (bsg_job->request_len <
+	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
+		return -EINVAL;
+
 	mutex_lock(&ha->optrom_mutex);
 	rval = qla28xx_do_validate_flash_image(bsg_job, &state);
 	if (rval)
Signature · 1 line
2.47.3
author maintainer
#31 lore raw

PATCH 31/33 scsi: qla2xxx: Zero SFP DMA buffer in FRU/I2C bsg handlers

In reply to Nilesh Javali

The FRU and I2C bsg handlers stage their transfer in a DMA_POOL_SIZE (256-byte) bounce buffer obtained from dma_pool_alloc(), which does not zero the allocation. They initialize only a few leading bytes before handing the buffer to qla2x00_write_sfp().

qla2x00_write_sfp() can override the transfer length with a user-supplied value:

	if (len == 1)
		opt |= BIT_0;
	if (opt & BIT_0)
		len = *sfp;

*sfp is the first byte of the (user-controlled) payload, so len can grow up to 255. The device then DMA-reads len bytes from the 256-byte pool buffer. Since only a small prefix was written
(e.g. MAX_FRU_SIZE == 36 bytes for a FRU version, one byte for a FRU status register), the hardware reads past the initialized region and writes up to ~219 bytes of stale DMA-pool heap memory to the device flash.

Allocate the buffer with dma_pool_zalloc() in all five FRU/I2C handlers so any bytes beyond the initialized data are zero rather than stale heap contents.

Fixes 697a4bc69159 ("[SCSI] qla2xxx: Provide method for updating I2C attached VPD.")
Fixes 9ebb5d9c69f1 ("[SCSI] qla2xxx: Add I2C BSG interface.")
---
 drivers/scsi/qla2xxx/qla_bsg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/scsi/qla2xxx/qla_bsg.c +5 -5
index 7fe7480fdc1b..59fcc726b682 100644
@@ -2008,7 +2008,7 @@ qla2x00_update_fru_versions(struct bsg_job *bsg_job)
 	struct qla_image_version *image;
 	uint32_t count;
 	dma_addr_t sfp_dma;
-	void *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
+	void *sfp = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
 
 	if (!sfp) {
 		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
@@ -2066,7 +2066,7 @@ qla2x00_read_fru_status(struct bsg_job *bsg_job)
 	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_status_reg *sr = (void *)bsg;
 	dma_addr_t sfp_dma;
-	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
+	uint8_t *sfp = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
 
 	if (!sfp) {
 		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
@@ -2117,7 +2117,7 @@ qla2x00_write_fru_status(struct bsg_job *bsg_job)
 	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_status_reg *sr = (void *)bsg;
 	dma_addr_t sfp_dma;
-	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
+	uint8_t *sfp = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
 
 	if (!sfp) {
 		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
@@ -2164,7 +2164,7 @@ qla2x00_write_i2c(struct bsg_job *bsg_job)
 	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_i2c_access *i2c = (void *)bsg;
 	dma_addr_t sfp_dma;
-	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
+	uint8_t *sfp = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
 
 	if (!sfp) {
 		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
@@ -2210,7 +2210,7 @@ qla2x00_read_i2c(struct bsg_job *bsg_job)
 	uint8_t bsg[DMA_POOL_SIZE] = {};
 	struct qla_i2c_access *i2c = (void *)bsg;
 	dma_addr_t sfp_dma;
-	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
+	uint8_t *sfp = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
 
 	if (!sfp) {
 		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
Signature · 1 line
2.47.3
author maintainer
#32 lore raw

PATCH 32/33 scsi: qla2xxx: Bound i2c->length in I2C bsg handlers

In reply to Nilesh Javali

struct qla_i2c_access carries a 16-bit length field alongside a fixed 64-byte buffer:

	struct qla_i2c_access {
		uint16_t device, offset, option, length;
		uint8_t  buffer[0x40];
	} __packed;

qla2x00_write_i2c() and qla2x00_read_i2c() use the user-supplied i2c->length without any bounds check. i2c is overlaid on a 256-byte on-stack buffer and sfp is a 256-byte DMA-pool buffer, so a length up to 65535 overruns both:

- write: memcpy(sfp, i2c->buffer, i2c->length) over-reads the stack and over-writes the sfp heap buffer, and qla2x00_write_sfp() then DMAs i2c->length bytes out of the 256-byte buffer.
- read: qla2x00_read_sfp() DMAs i2c->length bytes into the 256-byte sfp, then memcpy(i2c->buffer, sfp, i2c->length) overflows the 64-byte buffer inside the on-stack array.

A caller holding CAP_SYS_RAWIO can use this to corrupt the heap and the kernel stack. Reject requests whose length exceeds the buffer before any copy or DMA transfer in both handlers.

Fixes 9ebb5d9c69f1 ("[SCSI] qla2xxx: Add I2C BSG interface.")
---
 drivers/scsi/qla2xxx/qla_bsg.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/scsi/qla2xxx/qla_bsg.c +12 -0
index 59fcc726b682..ab559048dbb8 100644
@@ -2175,6 +2175,12 @@ qla2x00_write_i2c(struct bsg_job *bsg_job)
 	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
 	    bsg_job->request_payload.sg_cnt, i2c, sizeof(*i2c));
 
+	if (i2c->length > sizeof(i2c->buffer)) {
+		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
+		    EXT_STATUS_INVALID_PARAM;
+		goto dealloc;
+	}
+
 	memcpy(sfp, i2c->buffer, i2c->length);
 	rval = qla2x00_write_sfp(vha, sfp_dma, sfp,
 	    i2c->device, i2c->offset, i2c->length, i2c->option);
@@ -2221,6 +2227,12 @@ qla2x00_read_i2c(struct bsg_job *bsg_job)
 	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
 	    bsg_job->request_payload.sg_cnt, i2c, sizeof(*i2c));
 
+	if (i2c->length > sizeof(i2c->buffer)) {
+		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
+		    EXT_STATUS_INVALID_PARAM;
+		goto dealloc;
+	}
+
 	rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
 		i2c->device, i2c->offset, i2c->length, i2c->option);
 
Signature · 1 line
2.47.3
author maintainer
#33 lore raw

PATCH 33/33 scsi: qla2xxx: Update version to 12.00.00.2607b2

In reply to Nilesh Javali
---
 drivers/scsi/qla2xxx/qla_version.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_version.h +1 -1
index 1c0b01d70350..ef9dc88221c9 100644
@@ -6,7 +6,7 @@
 /*
  * Driver version
  */
-#define QLA2XXX_VERSION      "12.00.00.2607b1"
+#define QLA2XXX_VERSION      "12.00.00.2607b2"
 
 #define QLA_DRIVER_MAJOR_VER	12
 #define QLA_DRIVER_MINOR_VER	00
Signature · 1 line
2.47.3
#34 lore raw
In reply to Nilesh Javali

Nilesh,

Quoted 5 lines

This series collects bug fixes, hardening, and small cleanups for the qla2xxx driver that are independent of the QLA29xx adapter enablement. Most were uncovered by static analysis and fuzzing of the driver's interrupt, mailbox, NVMe, and BSG paths; 30 of the 33 patches carry a
Fixes: tag and are marked for stable.

Applied to 7.3/scsi-staging, thanks!

Signature · 1 line
Martin K. Petersen