[PATCH v2 0/2] scsi: qla4xxx: bound and correct the ISP83xx reset template

[PATCH v2 0/2] scsi: qla4xxx: bound and correct the ISP83xx reset template

[Yehyeong Lee]

v1 bounded the ISP83xx reset template read and walk. The review of that
patch raised two further problems, fixed here as a short series.

Patch 1 is the v1 fix with the template size and the per-entry geometry
read through le16_to_cpu() where the bounds are computed. On big-endian
v1 validated a byte-swapped size while the copy and the walk advanced by
the raw value, so the bound did not hold; the two now agree on both
endiannesses. No functional change on little-endian.

Patch 2 fixes the reset-sequence array cursor in
qla4_83xx_poll_read_list() that the same review noted. The cursor is
kept in a local and only written back when it reaches the end of the
array, so every read overwrites the same slot and a later
read-modify-write entry picks up a stale value. This is a correctness
fix; the index stays inside the array.

v1: https://lore.kernel.org/r/20261010181139.1911817-1-yhlee@isslab.korea.ac.kr

Yehyeong Lee (2):
  scsi: qla4xxx: bound the ISP83xx reset template to its buffer
  scsi: qla4xxx: advance the reset-sequence array cursor in
    poll_read_list

 drivers/scsi/qla4xxx/ql4_83xx.c | 117 ++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] scsi: qla4xxx: bound the ISP83xx reset template to its buffer

[Yehyeong Lee]

qla4_83xx_read_reset_template() reads the reset template out of the
adapter flash into a fixed buffer:

	ha->reset_tmplt.buff = vmalloc(QLA83XX_RESTART_TEMPLATE_SIZE);

It then takes the template length from the header it just read and uses
it to size the second flash read:

	tmplt_hdr_def_size = (ha->reset_tmplt.hdr->size -
	    ha->reset_tmplt.hdr->hdr_size) / sizeof(uint32_t);
	ret_val = qla4_83xx_flash_read_u32(ha, addr, p_buff,
	                                   tmplt_hdr_def_size);

hdr->size is a 16-bit field supplied by the flash contents and nothing
bounds it: QLA83XX_RESTART_TEMPLATE_SIZE appears only at the allocation
and is never compared against anything. qla4_83xx_flash_read_u32() walks
its word count storing one dword at a time with no end check, so a
template claiming a size larger than the 8192-byte buffer stores up to
~57 KB past it with flash-chosen contents. The checksum test runs after
the copy and cannot prevent this.

  BUG: KASAN: vmalloc-out-of-bounds in qla4_83xx_flash_read_u32 [qla4xxx]
  Write of size 4 at addr ffffc90000057000 by task init/1

qla4_83xx_process_reset_template() then walks the template with
hdr->entries as the loop bound and each entry's own size as the stride,
and the per-opcode handlers read hdr->count items from an entry. These
values all come from flash and none is checked against the end of the
template, so processing a crafted template reads past the buffer:

  BUG: KASAN: vmalloc-out-of-bounds in qla4_83xx_process_reset_template [qla4xxx]
  Read of size 4 at addr ffffc90000057004 by task init/1

A read-modify-write entry adds one more: its 8-bit index_a flash value
subscripts reset_tmplt.array[], which holds QLA83XX_MAX_RESET_SEQ_ENTRIES
(16) entries inside struct scsi_qla_host. Only 404 bytes follow the
array in that structure, so an index of 101 or more reads past the
allocation:

  BUG: KASAN: slab-out-of-bounds in qla4_83xx_rmw_crb_reg [qla4xxx]
  Read of size 4 at addr ffff88810644aa28 by task init/1

Bound the template size to the buffer before the copy, keep the entry
walk and each handler's item loop inside the template, and reject an
out-of-range array index. These sizes and counts are little-endian
16-bit flash fields; read them through le16_to_cpu() where the bounds
are computed, so that on a big-endian host the check does not run on a
byte-swapped size while the copy and the walk use the raw one.

While here, fix the state the error path leaves behind. It frees the
buffer without clearing the pointer, so qla4xxx_free_adapter() frees it
a second time on remove, and hdr is assigned before the header is
validated, so it keeps pointing into the freed buffer and a later
adapter reset dereferences it in qla4_83xx_process_reset_template().
Clear the template state on the error path and refuse to walk a
template that is not there.

Fixes: 6e7b429259fc0 ("[SCSI] qla4xxx: Added support for ISP83XX")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/qla4xxx/ql4_83xx.c | 114 ++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
index db41d90a5b6e9..44d285af22727 100644
--- a/drivers/scsi/qla4xxx/ql4_83xx.c
+++ b/drivers/scsi/qla4xxx/ql4_83xx.c
@@ -745,7 +745,8 @@ static int qla4_83xx_reset_seq_checksum_test(struct scsi_qla_host *ha)
 {
 	uint32_t sum =  0;
 	uint16_t *buff = (uint16_t *)ha->reset_tmplt.buff;
-	int u16_count =  ha->reset_tmplt.hdr->size / sizeof(uint16_t);
+	int u16_count =  le16_to_cpu(ha->reset_tmplt.hdr->size) /
+			 sizeof(uint16_t);
 	int ret_val;
 
 	while (u16_count-- > 0)
@@ -773,7 +774,7 @@ static int qla4_83xx_reset_seq_checksum_test(struct scsi_qla_host *ha)
 void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
 {
 	uint8_t *p_buff;
-	uint32_t addr, tmplt_hdr_def_size, tmplt_hdr_size;
+	uint32_t addr, tmplt_hdr_def_size, tmplt_hdr_size, tmplt_size;
 	uint32_t ret_val;
 
 	ha->reset_tmplt.seq_error = 0;
@@ -807,18 +808,36 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
 		(struct qla4_83xx_reset_template_hdr *)ha->reset_tmplt.buff;
 
 	/* Validate the template header size and signature */
-	tmplt_hdr_size = ha->reset_tmplt.hdr->hdr_size/sizeof(uint32_t);
+	tmplt_hdr_size = le16_to_cpu(ha->reset_tmplt.hdr->hdr_size) /
+			 sizeof(uint32_t);
 	if ((tmplt_hdr_size != tmplt_hdr_def_size) ||
-	    (ha->reset_tmplt.hdr->signature != RESET_TMPLT_HDR_SIGNATURE)) {
+	    (le16_to_cpu(ha->reset_tmplt.hdr->signature) !=
+	     RESET_TMPLT_HDR_SIGNATURE)) {
 		ql4_printk(KERN_ERR, ha, "%s: Template Header size %d is invalid, tmplt_hdr_def_size %d\n",
 			   __func__, tmplt_hdr_size, tmplt_hdr_def_size);
 		goto exit_read_template_error;
 	}
 
-	addr = QLA83XX_RESET_TEMPLATE_ADDR + ha->reset_tmplt.hdr->hdr_size;
-	p_buff = ha->reset_tmplt.buff + ha->reset_tmplt.hdr->hdr_size;
-	tmplt_hdr_def_size = (ha->reset_tmplt.hdr->size -
-			      ha->reset_tmplt.hdr->hdr_size) / sizeof(uint32_t);
+	/* The template size is chosen by the flash contents and nothing has
+	 * bounded it yet, so a template claiming to be larger than the buffer
+	 * would make the read below store past the end of it.  The integrity
+	 * check runs after the copy and cannot prevent that.
+	 */
+	tmplt_size = le16_to_cpu(ha->reset_tmplt.hdr->size);
+	if (tmplt_size > QLA83XX_RESTART_TEMPLATE_SIZE ||
+	    tmplt_size < le16_to_cpu(ha->reset_tmplt.hdr->hdr_size)) {
+		ql4_printk(KERN_ERR, ha, "%s: Template size %u is invalid\n",
+			   __func__, tmplt_size);
+		goto exit_read_template_error;
+	}
+
+	addr = QLA83XX_RESET_TEMPLATE_ADDR +
+	       le16_to_cpu(ha->reset_tmplt.hdr->hdr_size);
+	p_buff = ha->reset_tmplt.buff +
+		 le16_to_cpu(ha->reset_tmplt.hdr->hdr_size);
+	tmplt_hdr_def_size = (tmplt_size -
+			      le16_to_cpu(ha->reset_tmplt.hdr->hdr_size)) /
+			     sizeof(uint32_t);
 
 	DEBUG2(ql4_printk(KERN_INFO, ha,
 			  "%s: Read rest of the template size %d\n",
@@ -845,17 +864,22 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
 
 	/* Get STOP, START, INIT sequence offsets */
 	ha->reset_tmplt.init_offset = ha->reset_tmplt.buff +
-				      ha->reset_tmplt.hdr->init_seq_offset;
+		le16_to_cpu(ha->reset_tmplt.hdr->init_seq_offset);
 	ha->reset_tmplt.start_offset = ha->reset_tmplt.buff +
-				       ha->reset_tmplt.hdr->start_seq_offset;
+		le16_to_cpu(ha->reset_tmplt.hdr->start_seq_offset);
 	ha->reset_tmplt.stop_offset = ha->reset_tmplt.buff +
-				      ha->reset_tmplt.hdr->hdr_size;
+		le16_to_cpu(ha->reset_tmplt.hdr->hdr_size);
 	qla4_83xx_dump_reset_seq_hdr(ha);
 
 	goto exit_read_reset_template;
 
 exit_read_template_error:
 	vfree(ha->reset_tmplt.buff);
+	ha->reset_tmplt.buff = NULL;
+	ha->reset_tmplt.hdr = NULL;
+	ha->reset_tmplt.init_offset = NULL;
+	ha->reset_tmplt.start_offset = NULL;
+	ha->reset_tmplt.stop_offset = NULL;
 
 exit_read_reset_template:
 	return;
@@ -894,6 +918,13 @@ static void qla4_83xx_rmw_crb_reg(struct scsi_qla_host *ha, uint32_t raddr,
 {
 	uint32_t value;
 
+	if (p_rmw_hdr->index_a >= QLA83XX_MAX_RESET_SEQ_ENTRIES) {
+		ql4_printk(KERN_ERR, ha, "%s: Invalid array index %u\n",
+			   __func__, p_rmw_hdr->index_a);
+		ha->reset_tmplt.seq_error++;
+		return;
+	}
+
 	if (p_rmw_hdr->index_a)
 		value = ha->reset_tmplt.array[p_rmw_hdr->index_a];
 	else
@@ -914,12 +945,14 @@ static void qla4_83xx_write_list(struct scsi_qla_host *ha,
 				 struct qla4_83xx_reset_entry_hdr *p_hdr)
 {
 	struct qla4_83xx_entry *p_entry;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 
 	p_entry = (struct qla4_83xx_entry *)
 		  ((char *)p_hdr + sizeof(struct qla4_83xx_reset_entry_hdr));
 
-	for (i = 0; i < p_hdr->count; i++, p_entry++) {
+	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 		qla4_83xx_wr_reg_indirect(ha, p_entry->arg1, p_entry->arg2);
 		if (p_hdr->delay)
 			udelay((uint32_t)(p_hdr->delay));
@@ -930,12 +963,14 @@ static void qla4_83xx_read_write_list(struct scsi_qla_host *ha,
 				      struct qla4_83xx_reset_entry_hdr *p_hdr)
 {
 	struct qla4_83xx_entry *p_entry;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 
 	p_entry = (struct qla4_83xx_entry *)
 		  ((char *)p_hdr + sizeof(struct qla4_83xx_reset_entry_hdr));
 
-	for (i = 0; i < p_hdr->count; i++, p_entry++) {
+	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 		qla4_83xx_read_write_crb_reg(ha, p_entry->arg1, p_entry->arg2);
 		if (p_hdr->delay)
 			udelay((uint32_t)(p_hdr->delay));
@@ -948,6 +983,7 @@ static void qla4_83xx_poll_list(struct scsi_qla_host *ha,
 	long delay;
 	struct qla4_83xx_entry *p_entry;
 	struct qla4_83xx_poll *p_poll;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 	uint32_t value;
 
@@ -961,13 +997,15 @@ static void qla4_83xx_poll_list(struct scsi_qla_host *ha,
 
 	delay = (long)p_hdr->delay;
 	if (!delay) {
-		for (i = 0; i < p_hdr->count; i++, p_entry++) {
+		for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+		     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 			qla4_83xx_poll_reg(ha, p_entry->arg1, delay,
 					   p_poll->test_mask,
 					   p_poll->test_value);
 		}
 	} else {
-		for (i = 0; i < p_hdr->count; i++, p_entry++) {
+		for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+		     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 			if (qla4_83xx_poll_reg(ha, p_entry->arg1, delay,
 					       p_poll->test_mask,
 					       p_poll->test_value)) {
@@ -986,6 +1024,7 @@ static void qla4_83xx_poll_write_list(struct scsi_qla_host *ha,
 	long delay;
 	struct qla4_83xx_quad_entry *p_entry;
 	struct qla4_83xx_poll *p_poll;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 
 	p_poll = (struct qla4_83xx_poll *)
@@ -994,7 +1033,8 @@ static void qla4_83xx_poll_write_list(struct scsi_qla_host *ha,
 		  ((char *)p_poll + sizeof(struct qla4_83xx_poll));
 	delay = (long)p_hdr->delay;
 
-	for (i = 0; i < p_hdr->count; i++, p_entry++) {
+	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 		qla4_83xx_wr_reg_indirect(ha, p_entry->dr_addr,
 					  p_entry->dr_value);
 		qla4_83xx_wr_reg_indirect(ha, p_entry->ar_addr,
@@ -1017,6 +1057,7 @@ static void qla4_83xx_read_modify_write(struct scsi_qla_host *ha,
 {
 	struct qla4_83xx_entry *p_entry;
 	struct qla4_83xx_rmw *p_rmw_hdr;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 
 	p_rmw_hdr = (struct qla4_83xx_rmw *)
@@ -1024,7 +1065,8 @@ static void qla4_83xx_read_modify_write(struct scsi_qla_host *ha,
 	p_entry = (struct qla4_83xx_entry *)
 		  ((char *)p_rmw_hdr + sizeof(struct qla4_83xx_rmw));
 
-	for (i = 0; i < p_hdr->count; i++, p_entry++) {
+	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 		qla4_83xx_rmw_crb_reg(ha, p_entry->arg1, p_entry->arg2,
 				      p_rmw_hdr);
 		if (p_hdr->delay)
@@ -1046,6 +1088,7 @@ static void qla4_83xx_poll_read_list(struct scsi_qla_host *ha,
 	int index;
 	struct qla4_83xx_quad_entry *p_entry;
 	struct qla4_83xx_poll *p_poll;
+	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
 	uint32_t i;
 	uint32_t value;
 
@@ -1055,7 +1098,8 @@ static void qla4_83xx_poll_read_list(struct scsi_qla_host *ha,
 		  ((char *)p_poll + sizeof(struct qla4_83xx_poll));
 	delay = (long)p_hdr->delay;
 
-	for (i = 0; i < p_hdr->count; i++, p_entry++) {
+	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
+	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
 		qla4_83xx_wr_reg_indirect(ha, p_entry->ar_addr,
 					  p_entry->ar_value);
 		if (delay) {
@@ -1117,16 +1161,44 @@ static void qla4_83xx_process_reset_template(struct scsi_qla_host *ha,
 	int index, entries;
 	struct qla4_83xx_reset_entry_hdr *p_hdr;
 	char *p_entry = p_buff;
+	char *p_end;
+
+	if (!ha->reset_tmplt.buff || !ha->reset_tmplt.hdr || !p_buff) {
+		ql4_printk(KERN_ERR, ha, "%s: No valid reset template\n",
+			   __func__);
+		return;
+	}
 
 	ha->reset_tmplt.seq_end = 0;
 	ha->reset_tmplt.template_end = 0;
-	entries = ha->reset_tmplt.hdr->entries;
+	entries = le16_to_cpu(ha->reset_tmplt.hdr->entries);
 	index = ha->reset_tmplt.seq_index;
+	p_end = (char *)ha->reset_tmplt.buff +
+		le16_to_cpu(ha->reset_tmplt.hdr->size);
 
 	for (; (!ha->reset_tmplt.seq_end) && (index  < entries); index++) {
 
+		/* Both the entry count and each entry size come from flash,
+		 * so stop rather than walk past the end of the template.
+		 */
+		if (p_entry < (char *)ha->reset_tmplt.buff ||
+		    p_entry + sizeof(*p_hdr) > p_end) {
+			ql4_printk(KERN_ERR, ha, "%s: Entry %d is outside the template\n",
+				   __func__, index);
+			ha->reset_tmplt.seq_error++;
+			break;
+		}
+
 		p_hdr = (struct qla4_83xx_reset_entry_hdr *)p_entry;
-		switch (p_hdr->cmd) {
+		if (le16_to_cpu(p_hdr->size) < sizeof(*p_hdr) ||
+		    p_entry + le16_to_cpu(p_hdr->size) > p_end) {
+			ql4_printk(KERN_ERR, ha, "%s: Entry %d size %u is outside the template\n",
+				   __func__, index, le16_to_cpu(p_hdr->size));
+			ha->reset_tmplt.seq_error++;
+			break;
+		}
+
+		switch (le16_to_cpu(p_hdr->cmd)) {
 		case OPCODE_NOP:
 			break;
 		case OPCODE_WRITE_LIST:
@@ -1163,7 +1235,7 @@ static void qla4_83xx_process_reset_template(struct scsi_qla_host *ha,
 		}
 
 		/* Set pointer to next entry in the sequence. */
-		p_entry += p_hdr->size;
+		p_entry += le16_to_cpu(p_hdr->size);
 	}
 
 	ha->reset_tmplt.seq_index = index;
-- 
2.43.0

Re: [PATCH v2 1/2] scsi: qla4xxx: bound the ISP83xx reset template to its buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] scsi: qla4xxx: missing endianness conversions for reset template payload fields
- [Medium] scsi: qla4xxx: reset sequence checksum check unconditionally passes
- [Low] scsi: qla4xxx: truncation in template size division causes uninitialized vmalloc memory read
--

commit 4074c809e0d6707e10d7f40671e94534928d0d52
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: qla4xxx: bound the ISP83xx reset template to its buffer

This commit bounds the ISP83xx reset template to its allocated buffer
before copying from flash, and validates sizes to prevent out-of-bounds
reads and writes. It also adds error path cleanups for the template
pointer states.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
> index db41d90a5b6e9..44d285af22727 100644
> --- a/drivers/scsi/qla4xxx/ql4_83xx.c
> +++ b/drivers/scsi/qla4xxx/ql4_83xx.c
> @@ -745,7 +745,8 @@ static int qla4_83xx_reset_seq_checksum_test(struct scsi_qla_host *ha)
>  {
>  	uint32_t sum =  0;
>  	uint16_t *buff = (uint16_t *)ha->reset_tmplt.buff;
> -	int u16_count =  ha->reset_tmplt.hdr->size / sizeof(uint16_t);
> +	int u16_count =  le16_to_cpu(ha->reset_tmplt.hdr->size) /
> +			 sizeof(uint16_t);
>  	int ret_val;
>  
>  	while (u16_count-- > 0)
>  		sum += *buff++;
>  
>  	while (sum >> 16)
>  		sum = (sum & 0xFFFF) +  (sum >> 16);
>  
>  	/* checksum of 0 indicates a valid template */
>  	if (~sum) {
>  		ret_val = QLA_SUCCESS;
>  	} else {

[Severity: Medium]
Because sum is a 32-bit unsigned integer with its upper 16 bits guaranteed
to be zero after the loop, won't ~sum always result in a value where the
upper 16 bits are set to 1?

Does this mean ~sum will always evaluate to true (non-zero), unconditionally
passing the checksum check regardless of the flash contents?

[ ... ]

> @@ -807,18 +808,36 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
[ ... ]
> +	tmplt_hdr_def_size = (tmplt_size -
> +			      le16_to_cpu(ha->reset_tmplt.hdr->hdr_size)) /
> +			     sizeof(uint32_t);

[Severity: Low]
If the template size minus the header size is not a multiple of 4, integer
division by sizeof(uint32_t) truncates the remaining bytes.

When qla4_83xx_flash_read_u32() reads this truncated word count, could it
leave up to 3 bytes uninitialized at the end of the buffer?

Later, when qla4_83xx_reset_seq_checksum_test() loops over u16_count (which
covers the full size), could it read those uninitialized bytes from the
vmalloc buffer into the checksum calculation?

[ ... ]

> @@ -914,12 +945,14 @@ static void qla4_83xx_write_list(struct scsi_qla_host *ha,
>  				 struct qla4_83xx_reset_entry_hdr *p_hdr)
>  {
>  	struct qla4_83xx_entry *p_entry;
> +	char *p_end = (char *)p_hdr + le16_to_cpu(p_hdr->size);
>  	uint32_t i;
>  
>  	p_entry = (struct qla4_83xx_entry *)
>  		  ((char *)p_hdr + sizeof(struct qla4_83xx_reset_entry_hdr));
>  
> -	for (i = 0; i < p_hdr->count; i++, p_entry++) {
> +	for (i = 0; i < le16_to_cpu(p_hdr->count) &&
> +	     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
>  		qla4_83xx_wr_reg_indirect(ha, p_entry->arg1, p_entry->arg2);
>  		if (p_hdr->delay)
>  			udelay((uint32_t)(p_hdr->delay));

[Severity: Medium]
The patch correctly adds le16_to_cpu() for bounds checking like p_hdr->count,
but the payload fields (arg1, arg2, delay) are used as raw values.

Could this regression write garbage to the hardware registers and apply
incorrect delays on big-endian systems because the little-endian values
aren't converted to host byte order?

[ ... ]

> @@ -948,6 +983,7 @@ static void qla4_83xx_poll_list(struct scsi_qla_host *ha,
[ ... ]
>  	delay = (long)p_hdr->delay;
>  	if (!delay) {
> -		for (i = 0; i < p_hdr->count; i++, p_entry++) {
> +		for (i = 0; i < le16_to_cpu(p_hdr->count) &&
> +		     (char *)(p_entry + 1) <= p_end; i++, p_entry++) {
>  			qla4_83xx_poll_reg(ha, p_entry->arg1, delay,
>  					   p_poll->test_mask,
>  					   p_poll->test_value);

[Severity: Medium]
Similarly here, test_mask and test_value are used without endianness swapping.

Will this regression cause the polling loop logic to compare against swapped
bits on big-endian architectures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011040257.2017421-1-yhlee@isslab.korea.ac.kr?part=1

[PATCH v2 2/2] scsi: qla4xxx: advance the reset-sequence array cursor in poll_read_list

[Yehyeong Lee]

qla4_83xx_poll_read_list() stashes each value it reads into
ha->reset_tmplt.array[] and is meant to advance the cursor so the next
read lands in the next slot, which a later read-modify-write entry then
picks up by index:

	index = ha->reset_tmplt.array_index;
	qla4_83xx_rd_reg_indirect(ha, p_entry->dr_addr, &value);
	ha->reset_tmplt.array[index++] = value;
	if (index == QLA83XX_MAX_RESET_SEQ_ENTRIES)
		ha->reset_tmplt.array_index = 1;

index is a local copy and is only written back to array_index when it
reaches the end of the array. Every other read reloads the same cursor
and overwrites the same slot, so the array is never filled past its
first entry and a read-modify-write entry referring to a later index
gets a stale or zero value and programs the wrong word into the
hardware during reset recovery.

Write the cursor back on every read so the slots fill in order.

Fixes: 6e7b429259fc0 ("[SCSI] qla4xxx: Added support for ISP83XX")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/qla4xxx/ql4_83xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
index 44d285af22727..2ac833c5b1400 100644
--- a/drivers/scsi/qla4xxx/ql4_83xx.c
+++ b/drivers/scsi/qla4xxx/ql4_83xx.c
@@ -1117,7 +1117,8 @@ static void qla4_83xx_poll_read_list(struct scsi_qla_host *ha,
 				ha->reset_tmplt.array[index++] = value;
 
 				if (index == QLA83XX_MAX_RESET_SEQ_ENTRIES)
-					ha->reset_tmplt.array_index = 1;
+					index = 1;
+				ha->reset_tmplt.array_index = index;
 			}
 		}
 	}
-- 
2.43.0

Re: [PATCH v2 2/2] scsi: qla4xxx: advance the reset-sequence array cursor in poll_read_list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla4xxx: missing initialization of reset template array_index corrupts firmware restart
--

commit f2bbbc943d62eae5bbd2ca3ef4c8394aabc05ad4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: qla4xxx: advance the reset-sequence array cursor in poll_read_list

This commit fixes qla4_83xx_poll_read_list() to ensure the array_index
cursor is saved back to ha->reset_tmplt on every read. This prevents
multiple reads from overwriting the same slot during the hardware reset
sequence.

> diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
> index 44d285af22727..2ac833c5b1400 100644
> --- a/drivers/scsi/qla4xxx/ql4_83xx.c
> +++ b/drivers/scsi/qla4xxx/ql4_83xx.c
> @@ -1117,7 +1117,8 @@ static void qla4_83xx_poll_read_list(struct scsi_qla_host *ha,
>  				ha->reset_tmplt.array[index++] = value;
>  
>  				if (index == QLA83XX_MAX_RESET_SEQ_ENTRIES)
> -					ha->reset_tmplt.array_index = 1;
> +					index = 1;
> +				ha->reset_tmplt.array_index = index;

[Severity: High]
Since struct scsi_qla_host is zero-initialized, ha->reset_tmplt.array_index
starts at 0. During the first execution of qla4_83xx_poll_read_list(), the
polled value will be placed at array[0].

However, looking at qla4_83xx_rmw_crb_reg(), an index of 0 is treated as a
hardware bypass flag to read the register directly instead of the array:

qla4_83xx_rmw_crb_reg() {
	...
	if (p_rmw_hdr->index_a)
		value = ha->reset_tmplt.array[p_rmw_hdr->index_a];
	else
		qla4_83xx_rd_reg_indirect(ha, raddr, &value);
	...
}

Does this make array[0] completely inaccessible to the template? The wrap
around check setting index = 1 suggests the template logic assumes 1-based
indexing. If the first polled value is written to array[0], won't all later
read-modify-write operations read from the wrong array slot?

Additionally, when attempting firmware recovery via qla4_83xx_restart(),
qla4_83xx_process_stop_seq() resets ha->reset_tmplt.seq_index to 0:

qla4_83xx_process_stop_seq() {
	ha->reset_tmplt.seq_index = 0;
	qla4_83xx_process_reset_template(ha, ha->reset_tmplt.stop_offset);
}

But it appears to omit resetting ha->reset_tmplt.array_index to 1. On
subsequent reset attempts, does this cause the cursor to drift continually
rather than starting fresh, completely desynchronizing the polled values
from the template execution?

>  			}
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011040257.2017421-1-yhlee@isslab.korea.ac.kr?part=2