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

[PATCH v4] 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 smaller than the fixed header size of
struct fsf_status_read_buffer, subtracting the payload offset
underflows and can lead to an out-of-bounds read.

zfcp_dbf_san_in_els() has the same issue, where the underflowed value
is used as the scatterlist payload length.

Fix both cases by validating srb->length before subtracting the payload
offset. If the reported buffer length does not reach the payload area,
prevent payload access and treat the payload as empty.

For SAN tracing, retain the original channel-reported SRB length in the
trace record when no valid payload area exists. This preserves trace
evidence of anomalous firmware-reported lengths for serviceability,
while using a capture length of zero to ensure that no payload data is
accessed beyond the reported buffer.

For valid payloads, preserve the existing tracing behavior.

Fixes: a54ca0f62f95 ("[SCSI] zfcp: Redesign of the debug tracing for HBA records.")
Fixes: 2c55b750a884 ("[SCSI] zfcp: Redesign of the debug tracing for SAN records.")
Signed-off-by: Ajaykumar Rajappa <ajaykr@linux.ibm.com>
---
Changes in v4:
 - Preserve the original channel-reported SRB length in SAN trace
   records when no payload area exists.
 - Distinguish between the recorded length and the payload length that
   may be accessed using length/cap_length semantics.
 - Use a capture length of zero to prevent payload access for malformed
   status read buffers.
 - Address both the underflow/OOB issue and the trace serviceability
   concern in the same patch. 
Changes in v3: 
 - Rework payload length handling for srb->length < pay_offset and 
   treat such cases as an empty payload.
 - Prevent payload processing and scatterlist initialization when the 
   reported buffer length does not reach the payload area.
 - Keep the fix focused on the underflow/OOB issue and preserve the 
   existing tracing behavior for valid payloads.
Changes in v2: 
 - Preserve anomalous short srb->length values by capping payload lengths
   to srb->length instead of forcing them to 0.
 - Introduce a local pay_offs₹et variable to avoid repeated offsetof()

Changes on the git hub can be found at: 
https://github.ibm.com/Ajay-Kumar-K-R/linux/tree/zfcp-fix-srb-length-underflow-v4
---
 drivers/s390/scsi/zfcp_dbf.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..d487e9bb1561 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,9 @@ 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,
@@ -714,17 +716,34 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
 	struct zfcp_dbf *dbf = fsf->adapter->dbf;
 	struct fsf_status_read_buffer *srb =
 		(struct fsf_status_read_buffer *) fsf->data;
-	u16 length;
+	u16 length, cap_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);
+	if (srb->length < pay_offset) {
+		/*
+		 * Invalid buffer: record the reported SRB length in pl_len
+		 * for tracing, but capture no payload.
+		 */
+		length = srb->length;
+		cap_length = 0;
+	} else {
+		/*
+		 * Valid buffer: record the payload length in pl_len
+		 * and capture the payload.
+		 */
+		length = (u16)(srb->length - pay_offset);
+		cap_length = length;
+ 
+		sg_init_one(&sg, srb->payload.data, length);
+	}
+
+	zfcp_dbf_san(tag, dbf, "san_els", cap_length ? &sg : NULL,
+		     ZFCP_DBF_SAN_ELS, length, fsf->req_id,
+		     ntoh24(srb->d_id), cap_length);
 }
 
 /**
-- 
2.53.0

Re: [PATCH v4] 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:
- [Low] scsi: zfcp: build failure due to stray non-breaking space
--

commit 2a8ab7a51f83082138fe5de204f20c12d0e7bf28
Author: Ajaykumar Rajappa <ajaykr@linux.ibm.com>

zfcp: Fix integer underflow in status read buffer payload length

This commit fixes an integer underflow in zfcp_dbf_hba_fsf_uss() and
zfcp_dbf_san_in_els() when srb->length is smaller than the fixed header
size, preventing an out-of-bounds read. It ensures payload access is skipped
if the buffer is too small, while maintaining the correct tracing semantics.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index 81fb8af408e9c..d487e9bb15617 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c

[ ... ]

> @@ -714,17 +716,34 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
>  	struct zfcp_dbf *dbf = fsf->adapter->dbf;
>  	struct fsf_status_read_buffer *srb =
>  		(struct fsf_status_read_buffer *) fsf->data;
> -	u16 length;
> +	u16 length, cap_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);
> +	if (srb->length < pay_offset) {
> +		/*
> +		 * Invalid buffer: record the reported SRB length in pl_len
> +		 * for tracing, but capture no payload.
> +		 */
> +		length = srb->length;
> +		cap_length = 0;
> +	} else {
> +		/*
> +		 * Valid buffer: record the payload length in pl_len
> +		 * and capture the payload.
> +		 */
> +		length = (u16)(srb->length - pay_offset);
> +		cap_length = length;
> + 
> +		sg_init_one(&sg, srb->payload.data, length);

[Severity: Low]
Does this code contain a stray non-breaking space character?

The blank line right above the sg_init_one() call in zfcp_dbf_san_in_els()
contains a non-breaking space (U+00A0). This will unconditionally cause a
compiler syntax error ('stray \302\240 in program') when building the
driver.

> +	}
> +
> +	zfcp_dbf_san(tag, dbf, "san_els", cap_length ? &sg : NULL,
> +		     ZFCP_DBF_SAN_ELS, length, fsf->req_id,
> +		     ntoh24(srb->d_id), cap_length);
>  }

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

[PATCH v4] 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 smaller than the fixed header size of
struct fsf_status_read_buffer, subtracting the payload offset
underflows and can lead to an out-of-bounds read.

zfcp_dbf_san_in_els() has the same issue, where the underflowed value
is used as the scatterlist payload length.

Fix both cases by validating srb->length before subtracting the payload
offset. If the reported buffer length does not reach the payload area,
prevent payload access and treat the payload as empty.

For SAN tracing, retain the original channel-reported SRB length in the
trace record when no valid payload area exists. This preserves trace
evidence of anomalous firmware-reported lengths for serviceability,
while using a capture length of zero to ensure that no payload data is
accessed beyond the reported buffer.

For valid payloads, preserve the existing tracing behavior.

Fixes: a54ca0f62f95 ("[SCSI] zfcp: Redesign of the debug tracing for HBA records.")
Fixes: 2c55b750a884 ("[SCSI] zfcp: Redesign of the debug tracing for SAN records.")
Signed-off-by: Ajaykumar Rajappa <ajaykr@linux.ibm.com>
---
Changes in v4:
 - Preserve the original channel-reported SRB length in SAN trace
   records when no payload area exists.
 - Distinguish between the recorded length and the payload length that
   may be accessed using length/cap_length semantics.
 - Use a capture length of zero to prevent payload access for malformed
   status read buffers.
 - Address both the underflow/OOB issue and the trace serviceability
   concern in the same patch. 
Changes in v3: 
 - Rework payload length handling for srb->length < pay_offset and 
   treat such cases as an empty payload.
 - Prevent payload processing and scatterlist initialization when the 
   reported buffer length does not reach the payload area.
 - Keep the fix focused on the underflow/OOB issue and preserve the 
   existing tracing behavior for valid payloads.
Changes in v2: 
 - Preserve anomalous short srb->length values by capping payload lengths
   to srb->length instead of forcing them to 0.
 - Introduce a local pay_offs₹et variable to avoid repeated offsetof()

Changes on the git hub can be found at: 
https://github.ibm.com/Ajay-Kumar-K-R/linux/tree/zfcp-fix-srb-length-underflow-v4
---
 drivers/s390/scsi/zfcp_dbf.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..d147064ab72f 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,9 @@ 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,
@@ -714,17 +716,34 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
 	struct zfcp_dbf *dbf = fsf->adapter->dbf;
 	struct fsf_status_read_buffer *srb =
 		(struct fsf_status_read_buffer *) fsf->data;
-	u16 length;
+	u16 length, cap_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);
+	if (srb->length < pay_offset) {
+		/*
+		 * Invalid buffer: record the reported SRB length in pl_len
+		 * for tracing, but capture no payload.
+		 */
+		length = srb->length;
+		cap_length = 0;
+	} else {
+		/*
+		 * Valid buffer: record the payload length in pl_len
+		 * and capture the payload.
+		 */
+		length = (u16)(srb->length - pay_offset);
+		cap_length = length;
+
+		sg_init_one(&sg, srb->payload.data, length);
+	}
+
+	zfcp_dbf_san(tag, dbf, "san_els", cap_length ? &sg : NULL,
+		     ZFCP_DBF_SAN_ELS, length, fsf->req_id,
+		     ntoh24(srb->d_id), cap_length);
 }
 
 /**
-- 
2.53.0

Re: [PATCH v4] 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 smaller than the fixed header size of
> struct fsf_status_read_buffer, subtracting the payload offset
> underflows and can lead to an out-of-bounds read.
> 
> zfcp_dbf_san_in_els() has the same issue, where the underflowed value
> is used as the scatterlist payload length.
> 
> Fix both cases by validating srb->length before subtracting the payload
> offset. If the reported buffer length does not reach the payload area,
> prevent payload access and treat the payload as empty.
> 
> For SAN tracing, retain the original channel-reported SRB length in the
> trace record when no valid payload area exists. This preserves trace
> evidence of anomalous firmware-reported lengths for serviceability,
> while using a capture length of zero to ensure that no payload data is
> accessed beyond the reported buffer.
> 
> For valid payloads, preserve the existing tracing behavior.
> 
> Fixes: a54ca0f62f95 ("[SCSI] zfcp: Redesign of the debug tracing for HBA records.")
> Fixes: 2c55b750a884 ("[SCSI] zfcp: Redesign of the debug tracing for SAN records.")
> 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/20261005094631.3361197-1-ajaykr@linux.ibm.com?part=1