Re: [PATCH v4 46/56] scsi: qla2xxx: edif: Fix NULL pointer deref in RX SA delete check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch incompletely fixes a type confusion bug caused by late firmware completions, leaving a wild pointer dereference and out-of-bounds write unhandled.
- [High] Non-SCSI commands that erroneously fall through to the SCSI completion path are leaked because their completion callback is never executed if the union alias evaluates to NULL.
--

--- Patch [46]: [PATCH v4 46/56] scsi: qla2xxx: edif: Fix NULL pointer deref in RX SA delete check ---
commit 05a47ccd14015571b07bc1e0befc11762885c911
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: edif: Fix NULL pointer deref in RX SA delete check
    
qla_chk_edif_rx_sa_delete_pending() obtains the SCSI command via
GET_CMD_SP(sp) and immediately dereferences cmd->sc_data_direction.
That command pointer can be NULL: the firmware may post a status
completion for a command that has already been returned or aborted.
The caller qla2x00_status_entry() acknowledges this on the very same
status path, re-fetching GET_CMD_SP(sp) and bailing out with the
"Command already returned" message when it is NULL -- but that check
runs only after qla_chk_edif_rx_sa_delete_pending() has already
dereferenced the pointer, so a NULL cmd crashes the kernel in
interrupt context.

Cc: stable@vger.kernel.org
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>

> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> index ade1d81785736..bfa520f936a24 100644
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c
> @@ -3540,6 +3540,9 @@ void qla_chk_edif_rx_sa_delete_pending(scsi_qla_host_t *vha,
>  	uint32_t handle;
>  	uint16_t sa_index;
>  
> +	if (!cmd)
> +		return;
> +

[Severity: Critical]
This is a pre-existing issue, but does this check fully protect against wild
pointers?

If a non-SCSI command (like a BSG or ELS command) is completed with a
STATUS_TYPE IOCB (e.g., due to a timeout or transport error), sp->type
might not be SRB_SCSI_CMD.

In that case, the union evaluation in GET_CMD_SP(sp) could yield a non-NULL
wild pointer, leading to an out-of-bounds read when accessing
cmd->sc_data_direction here:

	if (cmd->sc_data_direction != DMA_FROM_DEVICE)

And similarly, the caller in qla2x00_status_entry() could cast this wild
pointer as a valid scsi_cmnd and corrupt memory in scsi_set_resid().

Should we verify sp->type == SRB_SCSI_CMD before trusting GET_CMD_SP(sp)?

[Severity: High]
This is also a pre-existing issue, but does the NULL check in the caller
leak non-SCSI commands?

Looking at qla2x00_status_entry() in drivers/scsi/qla2xxx/qla_isr.c:

	cp = GET_CMD_SP(sp);
	if (cp == NULL) {
		...
		req->outstanding_cmds[handle] = NULL;
		return;
	}

If a non-SCSI command (like an ELS PLOGI/LOGO command) is submitted, and the
GET_CMD_SP(sp) union happens to evaluate to NULL (e.g., if the flags and
els_cmd fields are zero), this path returns early without calling sp->done().

Can this leak the command and cause threads waiting for completion to hang?

>  	handle = (uint32_t)LSW(sts24->handle);
>  
>  	/* find out if this status iosb is for a scsi read */

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