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

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 <[email protected]>

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/[email protected]?part=1