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

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

[Pengpeng Hou]

fc_fcp_resp() checks only the fixed response templates before using
device-declared response-info and sense lengths. A short response can
therefore pass the existing fixed-header check while the response-code
read or sense copy reaches beyond the current frame.

Validate the response-info span before reading its response code and
validate the combined response-info and sense spans before copying
sense data.

Fixes: 42e9a92fe6a9 ("[SCSI] libfc: A modular Fibre Channel library")
Cc: stable@vger.kernel.org
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/scsi/libfc/fc_fcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index a5139e43ca4c..6a1489be2617 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -882,6 +882,9 @@ static void fc_fcp_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 				if ((respl != FCP_RESP_RSP_INFO_LEN4) &&
 				    (respl != FCP_RESP_RSP_INFO_LEN8))
 					goto len_err;
+				if (fsp->wait_for_comp &&
+				    plen < sizeof(*fc_rp) + sizeof(*rp_ex) + respl)
+					goto len_err;
 				if (fsp->wait_for_comp) {
 					/* Abuse cdb_status for rsp code */
 					fsp->cdb_status = fc_rp_info->rsp_code;
@@ -897,6 +900,8 @@ static void fc_fcp_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 				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);
 			}
-- 
2.43.0

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

[sashiko-bot]

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