[PATCH v3] zfcp: Fix integer underflow in status read buffer payload length

[PATCH v3] zfcp: Fix integer underflow in status read buffer payload length

[Ajaykumar Rajappa]

zfcp_dbf_hba_fsf_uss() only guards against a zero-length status read
buffer. If srb->length is non-zero but smaller than the fixed header
size of struct fsf_status_read_buffer, subtracting the payload offset
underflows and can result in an out-of-bounds read from
srb->payload.data.

zfcp_dbf_san_in_els() has the same problem. The underflowed value is
used as the scatterlist payload length, potentially causing accesses
beyond the reported status read buffer.

Prevent both underflows by validating srb->length before subtracting the
payload offset. If the reported status read buffer length does not reach
the payload area, treat the payload as empty rather than performing the
subtraction. This avoids the unsigned underflow and ensures that no
payload data is processed or accessed beyond the reported buffer.

Skip scatterlist setup and payload tracing when no valid payload exists.
If the reported status read buffer length does not reach the payload
area, no scatterlist is initialized and zfcp_dbf_san() is called
without payload data, preventing any access beyond the reported buffer.

For valid payloads, preserve the existing tracing behavior by continuing
to initialize the scatterlist and pass the computed payload length to
zfcp_dbf_san().

Signed-off-by: Ajaykumar Rajappa <ajaykr@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_dbf.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..5ae1302b993e 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -223,6 +223,7 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	struct zfcp_dbf_hba *rec = &dbf->hba_buf;
 	static int const level = 2;
 	unsigned long flags;
+	const u32 pay_offset = offsetof(struct fsf_status_read_buffer, payload);
 
 	if (unlikely(!debug_level_enabled(dbf->hba, level)))
 		return;
@@ -254,8 +255,8 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	memcpy(&rec->u.uss.res4, &srb->res4, sizeof(rec->u.uss.res4));
 
 	/* status read buffer payload length */
-	rec->pl_len = (!srb->length) ? 0 : srb->length -
-			offsetof(struct fsf_status_read_buffer, payload);
+	rec->pl_len = (srb->length < pay_offset) ? 0 :
+		      (u16)(srb->length - pay_offset);
 
 	if (rec->pl_len)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
@@ -716,15 +717,19 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
 		(struct fsf_status_read_buffer *) fsf->data;
 	u16 length;
 	struct scatterlist sg;
+	const u32 pay_offset = offsetof(struct fsf_status_read_buffer, payload);
 
 	if (unlikely(!debug_level_enabled(dbf->san, ZFCP_DBF_SAN_LEVEL)))
 		return;
 
-	length = (u16)(srb->length -
-			offsetof(struct fsf_status_read_buffer, payload));
-	sg_init_one(&sg, srb->payload.data, length);
-	zfcp_dbf_san(tag, dbf, "san_els", &sg, ZFCP_DBF_SAN_ELS, length,
-		     fsf->req_id, ntoh24(srb->d_id), length);
+	length = (srb->length < pay_offset) ? 0 :
+		 (u16)(srb->length - pay_offset);
+
+	if (length)
+		sg_init_one(&sg, srb->payload.data, length);
+
+	zfcp_dbf_san(tag, dbf, "san_els", length ? &sg : NULL, ZFCP_DBF_SAN_ELS,
+		     length, fsf->req_id, ntoh24(srb->d_id), length);
 }
 
 /**
-- 
2.53.0

Re: [PATCH v3] zfcp: Fix integer underflow in status read buffer payload length

[sashiko-bot]

> zfcp_dbf_hba_fsf_uss() only guards against a zero-length status read
> buffer. If srb->length is non-zero but smaller than the fixed header
> size of struct fsf_status_read_buffer, subtracting the payload offset
> underflows and can result in an out-of-bounds read from
> srb->payload.data.
> 
> zfcp_dbf_san_in_els() has the same problem. The underflowed value is
> used as the scatterlist payload length, potentially causing accesses
> beyond the reported status read buffer.
> 
> Prevent both underflows by validating srb->length before subtracting the
> payload offset. If the reported status read buffer length does not reach
> the payload area, treat the payload as empty rather than performing the
> subtraction. This avoids the unsigned underflow and ensures that no
> payload data is processed or accessed beyond the reported buffer.
> 
> Skip scatterlist setup and payload tracing when no valid payload exists.
> If the reported status read buffer length does not reach the payload
> area, no scatterlist is initialized and zfcp_dbf_san() is called
> without payload data, preventing any access beyond the reported buffer.
> 
> For valid payloads, preserve the existing tracing behavior by continuing
> to initialize the scatterlist and pass the computed payload length to
> zfcp_dbf_san().
> 
> Signed-off-by: Ajaykumar Rajappa <ajaykr@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924122400.1185067-1-ajaykr@linux.ibm.com?part=1

[PATCH v3] zfcp: Fix integer underflow in status read buffer payload length

[Ajaykumar Rajappa]

zfcp_dbf_hba_fsf_uss() only guards against a zero-length status read
buffer. If srb->length is non-zero but smaller than the fixed header
size of struct fsf_status_read_buffer, subtracting the payload offset
underflows and can result in an out-of-bounds read from
srb->payload.data.

zfcp_dbf_san_in_els() has the same problem. The underflowed value is
used as the scatterlist payload length, potentially causing accesses
beyond the reported status read buffer.

Prevent both underflows by validating srb->length before subtracting the
payload offset. If the reported status read buffer length does not reach
the payload area, treat the payload as empty rather than performing the
subtraction. This avoids the unsigned underflow and ensures that no
payload data is processed or accessed beyond the reported buffer.

Skip scatterlist setup and payload tracing when no valid payload exists.
If the reported status read buffer length does not reach the payload
area, no scatterlist is initialized and zfcp_dbf_san() is called
without payload data, preventing any access beyond the reported buffer.

For valid payloads, continue to initialize the scatterlist and trace the
payload via zfcp_dbf_san(). Payload capture remains bounded by
ZFCP_DBF_PAY_MAX_REC, preserving existing trace behavior for valid
status read buffers.

Signed-off-by: Ajaykumar Rajappa <ajaykr@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_dbf.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..e90ccaec2697 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -223,6 +223,7 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	struct zfcp_dbf_hba *rec = &dbf->hba_buf;
 	static int const level = 2;
 	unsigned long flags;
+	const u32 pay_offset = offsetof(struct fsf_status_read_buffer, payload);
 
 	if (unlikely(!debug_level_enabled(dbf->hba, level)))
 		return;
@@ -254,8 +255,8 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	memcpy(&rec->u.uss.res4, &srb->res4, sizeof(rec->u.uss.res4));
 
 	/* status read buffer payload length */
-	rec->pl_len = (!srb->length) ? 0 : srb->length -
-			offsetof(struct fsf_status_read_buffer, payload);
+	rec->pl_len = (srb->length < pay_offset) ? 0 :
+		      (u16)(srb->length - pay_offset);
 
 	if (rec->pl_len)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
@@ -716,15 +717,20 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
 		(struct fsf_status_read_buffer *) fsf->data;
 	u16 length;
 	struct scatterlist sg;
+	const u32 pay_offset = offsetof(struct fsf_status_read_buffer, payload);
 
 	if (unlikely(!debug_level_enabled(dbf->san, ZFCP_DBF_SAN_LEVEL)))
 		return;
 
-	length = (u16)(srb->length -
-			offsetof(struct fsf_status_read_buffer, payload));
-	sg_init_one(&sg, srb->payload.data, length);
-	zfcp_dbf_san(tag, dbf, "san_els", &sg, ZFCP_DBF_SAN_ELS, length,
-		     fsf->req_id, ntoh24(srb->d_id), length);
+	length = (srb->length < pay_offset) ? 0 :
+		 (u16)(srb->length - pay_offset);
+
+	if (length)
+		sg_init_one(&sg, srb->payload.data, length);
+
+	zfcp_dbf_san(tag, dbf, "san_els", length ? &sg : NULL, ZFCP_DBF_SAN_ELS,
+		     length, fsf->req_id, ntoh24(srb->d_id),
+		     min_t(u16, length, ZFCP_DBF_PAY_MAX_REC));
 }
 
 /**
-- 
2.53.0

Re: [PATCH v3] zfcp: Fix integer underflow in status read buffer payload length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: zfcp: payload capture artificially truncated in zfcp_dbf_san_in_els()
--

commit 041b21a7756415603296020fe0feb75f0060af3c
Author: Ajaykumar Rajappa <ajaykr@linux.ibm.com>

zfcp: Fix integer underflow in status read buffer payload length

This commit prevents integer underflows when processing the status read
buffer payload length in zfcp_dbf_hba_fsf_uss() and zfcp_dbf_san_in_els().
It validates that the buffer length reaches the payload area before
subtraction and skips scatterlist setup when the payload is empty.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c

[ ... ]

> @@ -716,15 +717,20 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
>  		(struct fsf_status_read_buffer *) fsf->data;
>  	u16 length;
>  	struct scatterlist sg;
> +	const u32 pay_offset = offsetof(struct fsf_status_read_buffer, payload);
>  
>  	if (unlikely(!debug_level_enabled(dbf->san, ZFCP_DBF_SAN_LEVEL)))
>  		return;
>  
> -	length = (u16)(srb->length -
> -			offsetof(struct fsf_status_read_buffer, payload));
> -	sg_init_one(&sg, srb->payload.data, length);
> -	zfcp_dbf_san(tag, dbf, "san_els", &sg, ZFCP_DBF_SAN_ELS, length,
> -		     fsf->req_id, ntoh24(srb->d_id), length);
> +	length = (srb->length < pay_offset) ? 0 :
> +		 (u16)(srb->length - pay_offset);
> +
> +	if (length)
> +		sg_init_one(&sg, srb->payload.data, length);
> +
> +	zfcp_dbf_san(tag, dbf, "san_els", length ? &sg : NULL, ZFCP_DBF_SAN_ELS,
> +		     length, fsf->req_id, ntoh24(srb->d_id),
> +		     min_t(u16, length, ZFCP_DBF_PAY_MAX_REC));

[Severity: Medium]
Does this artificially truncate the debugging trace payload for incoming ELS
frames?

When an ELS frame larger than 256 bytes is received while tracing is active,
passing min_t(u16, length, ZFCP_DBF_PAY_MAX_REC) here limits the capture to
the first 256 bytes, discarding the remainder of the payload and potentially
losing important debugging information.

The commit message states:

"Payload capture remains bounded by ZFCP_DBF_PAY_MAX_REC, preserving
existing trace behavior for valid status read buffers."

However, the previous code passed length directly as the capture length
argument, allowing zfcp_dbf_san() to write multiple 256-byte trace records
in a loop to capture the entire payload. Could this be updated to preserve
the original behavior of capturing the full payload length?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924115204.1090452-1-ajaykr@linux.ibm.com?part=1