Re: [PATCH v2 12/13] scsi: qla2xxx: Fix purex IOCB copy size on non-29xx adapters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: qla2xxx: false bug fix claim and bad Fixes tag in qla24xx_copy_std_pkt()
--

commit 80556b286ead6fe5fe43e6fbf646d23656fb7c8a
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix purex IOCB copy size on non-29xx adapters

This patch replaces QLA_DEFAULT_PAYLOAD_SIZE with
sizeof(struct purex_entry_24xx) in qla24xx_copy_std_pkt() for non-29xx
adapters and removes the QLA_DEFAULT_PAYLOAD_SIZE macro definition.

>     scsi: qla2xxx: Fix purex IOCB copy size on non-29xx adapters
>     
>     qla24xx_copy_std_pkt() copies a single purex IOCB from the request/
>     response ring into a purex_item. The source entry size depends on the
>     adapter: 64 bytes (struct purex_entry_24xx) on 27xx/28xx and 128 bytes
>     (struct purex_entry_24xx_ext) on 29xx.
>     
>     For non-29xx adapters the copy size was set to QLA_DEFAULT_PAYLOAD_SIZE
>     (128), causing memcpy() to read 64 bytes past the end of the 64-byte
>     source IOCB in the ring. Copy sizeof(struct purex_entry_24xx) instead so
>     exactly one IOCB is copied on these adapters.
>     
>     Cc: stable@vger.kernel.org
>     Fixes: 150a29ebda52 ("scsi: qla2xxx: Enhance purex_entry handling for 29xx series")
>     Signed-off-by: Nilesh Javali <njavali@marvell.com>
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 97e2a1a9ce3f3..da5ca604d99eb 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -5005,7 +5005,6 @@ struct active_regions {
>  #define QLA_SET_DATA_RATE_NOLR	1
>  #define QLA_SET_DATA_RATE_LR	2 /* Set speed and initiate LR */
>  
> -#define QLA_DEFAULT_PAYLOAD_SIZE	64
>  #define QLA_MAX_IOCB_SIZE		128
>  /*
>   * This item might be allocated with a size > sizeof(struct purex_item).
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 7bae26d4404c2..2a4eaefdcefc2 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -1220,7 +1220,7 @@ static struct purex_item
>  	if (IS_QLA29XX(ha))
>  		copy_sz = sizeof(struct purex_entry_24xx_ext);
>  	else
> -		copy_sz = QLA_DEFAULT_PAYLOAD_SIZE;
> +		copy_sz = sizeof(struct purex_entry_24xx);

[Severity: Medium]
Is the commit description accurate about QLA_DEFAULT_PAYLOAD_SIZE being 128
and causing an out-of-bounds read in qla24xx_copy_std_pkt()?

As shown in the diff for drivers/scsi/qla2xxx/qla_def.h above,
QLA_DEFAULT_PAYLOAD_SIZE is defined as 64 (whereas QLA_MAX_IOCB_SIZE is 128),
and struct purex_entry_24xx in drivers/scsi/qla2xxx/qla_fw.h is also 64 bytes
(enforced by BUILD_BUG_ON(sizeof(struct purex_entry_24xx) != 64) in
drivers/scsi/qla2xxx/qla_os.c).

Commit 7db93e3c580d ("scsi: qla2xxx: Enhance purex_entry handling for 29xx
series") already updated qla24xx_copy_std_pkt() in
drivers/scsi/qla2xxx/qla_isr.c from copying sizeof(item->iocb) to copying
copy_sz (64 bytes via QLA_DEFAULT_PAYLOAD_SIZE on non-29xx adapters).
Replacing QLA_DEFAULT_PAYLOAD_SIZE with sizeof(struct purex_entry_24xx)
therefore does not change the copy size or runtime behavior.

Additionally, the commit hash 150a29ebda52 in the Fixes tag does not exist in
the upstream tree (the upstream commit with that subject is 7db93e3c580d).
Since this change is a cleanup with no functional impact, should the Fixes
and Cc: stable@vger.kernel.org tags be dropped and the commit message updated
to reflect that this is a cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023101600.1431179-1-njavali@marvell.com?part=12