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

[sashiko-bot@kernel.org]

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