Re: [PATCH] scsi: libfc: validate FCP response payload length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Processing of FCP_SNS_LEN_VAL and FCP_RESID_UNDER blindly assumes fsp->cmd is valid, leading to a NULL pointer dereference during malformed Task Management Function (TMF) responses.
--

--- Patch [1]: [PATCH] scsi: libfc: validate FCP response payload length ---
commit 62a5c62c9541198f09e35c7317ed519450b54ee2
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: libfc: validate FCP response payload length

This commit adds bounds checking for FCP response payloads. It ensures
that the response-info and sense lengths declared by the device do not
exceed the current frame length, preventing out-of-bounds memory accesses.

> diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
> index 5121272f28fd8..7e4c3c152f5d2 100644
> --- a/drivers/scsi/libfc/fc_fcp.c
> +++ b/drivers/scsi/libfc/fc_fcp.c

[ ... ]

> @@ -869,6 +872,8 @@ static void fc_fcp_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
>  			if (flags & FCP_SNS_LEN_VAL) {
>  				snsl = ntohl(rp_ex->fr_sns_len);
>  				if (snsl > SCSI_SENSE_BUFFERSIZE)
>  					snsl = SCSI_SENSE_BUFFERSIZE;
> +				if (plen < sizeof(*fc_rp) + sizeof(*rp_ex) + respl + snsl)
> +					goto len_err;
>  				memcpy(fsp->cmd->sense_buffer,
>  				       (char *)fc_rp_info + respl, snsl);

[Severity: Critical]
This is a pre-existing issue, but does this code lead to a null pointer
dereference if a malformed Task Management Function response is received?

When fc_eh_device_reset() allocates an fsp for a LUN reset, it leaves
fsp->cmd as NULL and sets fsp->wait_for_comp = 1.

If a target responds with a frame where FCP_SNS_LEN_VAL is set but
FCP_RSP_LEN_VAL is clear, the early return for TMF completions is bypassed
and execution falls through to process sense data.

Because fsp->cmd is NULL in this path, will the memcpy() above crash when
attempting to access fsp->cmd->sense_buffer?

>  			}
>  		}
>  		if (flags & (FCP_RESID_UNDER | FCP_RESID_OVER)) {
>  			if (plen < sizeof(*fc_rp) + sizeof(rp_ex->fr_resid))
>  				goto len_err;
>  			if (flags & FCP_RESID_UNDER) {
>  				fsp->scsi_resid = ntohl(rp_ex->fr_resid);
>  				/*
>  				 * The cmnd->underflow is the minimum number of
>  				 * bytes that must be transferred for this
>  				 * command.  Provided a sense condition is not
>  				 * present, make sure the actual amount
>  				 * transferred is at least the underflow value
>  				 * or fail.
>  				 */
>  				if (!(flags & FCP_SNS_LEN_VAL) &&
>  				    (fc_rp->fr_status == 0) &&
>  				    (scsi_bufflen(fsp->cmd) -
>  				     fsp->scsi_resid) < fsp->cmd->underflow)

[Severity: Critical]
This is a pre-existing issue, but similarly, if the response has
FCP_RESID_UNDER set but FCP_RSP_LEN_VAL clear, will checking
scsi_bufflen(fsp->cmd) and fsp->cmd->underflow also crash when fsp->cmd
is NULL?

>  					goto err;
>  				expected_len -= fsp->scsi_resid;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715084548.45278-1-pengpeng@iscas.ac.cn?part=1