[PATCH] scsi: qla2xxx: Fix buffer overrun in login template

[PATCH] scsi: qla2xxx: Fix buffer overrun in login template

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In qla_get_login_template(), 'q' points to plogi_els_payld.fl_csp
(offset 4), but the copy length is sizeof(struct fc_els_flogi) = 152
bytes. This makes cpu_to_be32_array() write 4 bytes past the end of
plogi_els_payld, corrupting the adjacent struct member.

Fix by pointing 'q' to the start of plogi_els_payld (offset 0), so the
152-byte copy fits exactly. The transfer length passed to firmware is
unchanged, so firmware compatibility is preserved.

The written bytes at offset 0-3 are never consumed: the only consumers
read from offsetof(fl_csp) onward (148 bytes) - qla_iocb.c copies via
offsetof(fl_csp) and qla_os.c reads fl_csp.sp_bb_cred. This contract was
clarified/normalized by commit 134f66959cd0 ("scsi: qla2xxx: Silence a
static checker warning"), which adjusted the read-side handling and
silenced static checker warnings. Moving q to the start of the struct
therefore does not affect any reader.

Fixes: 44f5a37d1e3e ("scsi: qla2xxx: Fix buffer-buffer credit extraction error")
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/scsi/qla2xxx/qla_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index e746c9274cde..7e50cff79b5c 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -5666,7 +5666,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha)
 		       "PLOGI ELS param read fail.\n");
 		return;
 	}
-	q = (__be32 *)&ha->plogi_els_payld.fl_csp;
+	q = (__be32 *)&ha->plogi_els_payld;
 
 	bp = (uint32_t *)ha->init_cb;
 	cpu_to_be32_array(q, bp, sz / 4);
-- 
2.25.1

Re: [PATCH] scsi: qla2xxx: Fix buffer overrun in login template

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The fix for the buffer overrun shifts the PLOGI payload by 4 bytes, corrupting the Fibre Channel Service Parameters and breaking connection establishment.
--

--- Patch [1]: [PATCH] scsi: qla2xxx: Fix buffer overrun in login template ---
commit a554cfebac5525fe6bc8fe781ea87856585aeee0
Author: Cao Guanghui <caoguanghui@kylinos.cn>

scsi: qla2xxx: Fix buffer overrun in login template

This patch aims to fix a buffer overrun by changing the destination
pointer for the PLOGI ELS payload copy from the fl_csp field (offset 4)
to the start of the fc_els_flogi structure (offset 0).

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index e746c9274cded..7e50cff79b5c6 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -5666,7 +5666,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha)
>  		       "PLOGI ELS param read fail.\n");
>  		return;
>  	}
> -	q = (__be32 *)&ha->plogi_els_payld.fl_csp;
> +	q = (__be32 *)&ha->plogi_els_payld;

[Severity: High]
Does this change shift the firmware's payload backwards by 4 bytes
relative to the struct fields?

The commit message indicates moving the pointer to the start of the
struct does not affect any reader. However, looking at the consumers of
this data, they explicitly expect the firmware payload to align at
offsetof(fl_csp).

For example, in qla24xx_els_dcmd2_iocb() the driver copies the data like
this:

drivers/scsi/qla2xxx/qla_iocb.c:qla24xx_els_dcmd2_iocb()
    memcpy(elsio->u.els_plogi.els_plogi_pyld->data,
           (void *)&ha->plogi_els_payld + offsetof(struct fc_els_flogi, fl_csp),
           sizeof(ha->plogi_els_payld) - offsetof(struct fc_els_flogi, fl_csp));

Similarly, qla24xx_process_purex_rdp() reads sp_bb_cred from within fl_csp:

drivers/scsi/qla2xxx/qla_os.c:qla24xx_process_purex_rdp()
    be16_to_cpu(ha->plogi_els_payld.fl_csp.sp_bb_cred);

If the firmware payload consists purely of the Service Parameters without
a 4-byte ELS header, pointing q to the start of the structure will write
the parameters into the fl_cmd field.

Would this cause consumers reading from offsetof(fl_csp) to skip the
first 4 bytes of the actual parameters and read misaligned data,
potentially breaking connection establishments?

>  
>  	bp = (uint32_t *)ha->init_cb;
>  	cpu_to_be32_array(q, bp, sz / 4);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807093429.102604-1-ghuicao@163.com?part=1