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

[sashiko-bot@kernel.org]

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