[PATCH] scsi: qla4xxx: bound the ISP83xx reset template to its buffer

[PATCH] 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.

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 | 82 +++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
index db41d90a5b6e9..7fbcc4e85b26a 100644
--- a/drivers/scsi/qla4xxx/ql4_83xx.c
+++ b/drivers/scsi/qla4xxx/ql4_83xx.c
@@ -773,7 +773,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;
@@ -815,6 +815,19 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
 		goto exit_read_template_error;
 	}
 
+	/* 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 + 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 -
@@ -856,6 +869,11 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
 
 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 +912,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 +939,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 +957,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 +977,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 +991,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 +1018,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 +1027,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 +1051,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 +1059,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 +1082,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 +1092,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,15 +1155,43 @@ 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;
 	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;
+		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 (p_hdr->cmd) {
 		case OPCODE_NOP:
 			break;
-- 
2.43.0

Re: [PATCH] scsi: qla4xxx: bound the ISP83xx reset template to its buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: qla4xxx: inconsistent endianness bypasses bounds check on BE
- [High] scsi: qla4xxx: reset template array_index is never incremented
--

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

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

This commit bounds the size of the ISP83xx reset template during adapter
initialization to prevent buffer overruns when reading from flash. It also
ensures bounds are respected during template execution and handles error
cleanup properly.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
> index db41d90a5b6e9..7fbcc4e85b26a 100644
> --- a/drivers/scsi/qla4xxx/ql4_83xx.c
> +++ b/drivers/scsi/qla4xxx/ql4_83xx.c
> @@ -815,6 +815,19 @@ void qla4_83xx_read_reset_template(struct scsi_qla_host *ha)
>  		goto exit_read_template_error;
>  	}
>  
> +	/* 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 + 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);

[Severity: High]
Does this calculation use the raw unswapped __le16 values?

On Big-Endian architectures, the new le16_to_cpu() check above validates
the swapped size. However, an attacker could provide flash values that
pass the swapped bounds check, but become massive when read directly
without swapping.

Could this bypass the bounds check and lead to the vmalloc-out-of-bounds
write this patch intends to fix?

[ ... ]

> @@ -1046,6 +1082,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 +1092,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);

[Severity: High]
Is the array_index properly updated during this loop in
qla4_83xx_poll_read_list()?

Looking at the loop body:

			} else {
				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;
			}

The local index is incremented, but is it ever written back to
ha->reset_tmplt.array_index when it is less than the max? It appears
subsequent loop iterations will fetch the same stale index and overwrite
the same array element, breaking the read-modify-write reset operations.

[ ... ]

> @@ -1117,15 +1155,43 @@ 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;
>  	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;
> +		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 (p_hdr->cmd) {

[Severity: High]
Should the pointer increment at the end of this loop also use
le16_to_cpu(p_hdr->size)?

Looking at the end of qla4_83xx_process_reset_template():

		/* Set pointer to next entry in the sequence. */
		p_entry += p_hdr->size;

Because the new bounds check correctly uses the swapped value, advancing
the pointer by the unswapped value on Big-Endian systems could still jump
out of bounds and read past the buffer.

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