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

[PATCH v5] 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 payload access against the payload offset.
If the reported buffer length does not reach the payload area, suppress
payload reads and avoid constructing a scatterlist for non-existent
payload data.
 
For HBA tracing, preserve serviceability by unconditionally storing the
value-shifted SRB length in pl_len. This retains a reversible mapping to
the original channel-reported length without changing the existing trace
record format, while payload data is only read when the reported buffer
length reaches the payload area.

For SAN tracing, preserve the original channel-reported SRB length in
the trace record and separately track the number of payload bytes that
may be safely read. This retains trace evidence of anomalous
firmware-reported lengths while preventing payload reads 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 v5:
 - Address Steffen's review comments regarding HBA/SAN trace servicability.
 - Preserve HBA pl_len semantics so trace readers can always assume the
   value-shifted subtraction.
 - Retain the original channel-reported SRB length in SAN trace records.
 - Use a separate capped length to control payload reads in the SAN path.
 - Prevent payload reads when the reported buffer length does not reach
   the payload area.
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 capped payload
   length using length/cap_len semantics.
 - Set cap_length to zero when no payload area exists to prevent
   payload reads beyond the reported buffer.
 - 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-v5
---
 drivers/s390/scsi/zfcp_dbf.c | 36 ++++++++++++++++++++++++++----------
 drivers/s390/scsi/zfcp_dbf.h |  2 +-
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..6c85f5598a55 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;
@@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	rec->u.uss.s_id = ntoh24(srb->s_id);
 	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);
+	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
+	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
+	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
+	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
+	 */
+	rec->pl_len = (u16)(srb->length - pay_offset);
 
-	if (rec->pl_len)
+	/* Only access payload bytes when srb->length actually covers them. */
+	if (srb->length > pay_offset)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
 				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
 log:
@@ -714,17 +719,28 @@ 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);
+	/*
+	 * Store the original channel-reported SRB length in pl_len.
+	 * cap_length specifies the number of payload bytes that may be read.
+	 */
+	cap_length = 0;
+	length = (u16)srb->length - pay_offset;
+	if (srb->length > pay_offset) {
+		/* Payload present beyond the fixed header. */
+		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);
 }
 
 /**
diff --git a/drivers/s390/scsi/zfcp_dbf.h b/drivers/s390/scsi/zfcp_dbf.h
index 79973fb24b1c..81b4d5b79efd 100644
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -310,7 +310,7 @@ struct zfcp_dbf_pay {
 	u8 counter;
 	char area[ZFCP_DBF_TAG_LEN];
 	u64 fsf_req_id;
-#define ZFCP_DBF_PAY_MAX_REC 0x100
+#define ZFCP_DBF_PAY_MAX_REC	4032
 	char data[ZFCP_DBF_PAY_MAX_REC];
 } __packed;
 
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: zfcp: missing upper bound check on srb->length causes OOB read
- [Low] scsi: zfcp: SAN trace records shifted payload length instead of original SRB length
--

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

    zfcp: Fix integer underflow in status read buffer payload length
    
    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.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index 81fb8af408e9c..6c85f5598a551 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
>  	rec->u.uss.s_id = ntoh24(srb->s_id);
>  	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);
> +	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
> +	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
> +	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
> +	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
> +	 */
> +	rec->pl_len = (u16)(srb->length - pay_offset);
>  
> -	if (rec->pl_len)
> +	/* Only access payload bytes when srb->length actually covers them. */
> +	if (srb->length > pay_offset)
>  		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
>  				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);

[Severity: High]
Does srb->length need an upper bound check in zfcp_dbf_hba_fsf_uss() here?

If the firmware reports a large length (e.g., 65535), rec->pl_len will be
around 65471. Since the status read buffer is only 4096 bytes in size,
wouldn't this cause zfcp_dbf_pl_write() to read out of bounds past the page
boundary and potentially leak adjacent kernel memory into the trace buffer?

[ ... ]

> @@ -714,17 +719,28 @@ 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);
> +	/*
> +	 * Store the original channel-reported SRB length in pl_len.
> +	 * cap_length specifies the number of payload bytes that may be read.
> +	 */
> +	cap_length = 0;
> +	length = (u16)srb->length - pay_offset;
> +	if (srb->length > pay_offset) {
> +		/* Payload present beyond the fixed header. */
> +		cap_length = length;
> +		sg_init_one(&sg, srb->payload.data, length);
> +	}

[Severity: High]
Does this length also need an upper bound check in zfcp_dbf_san_in_els()
for the same reason?

If srb->length is large, this could initialize the scatterlist with an
unbounded length derived directly from firmware.

> +
> +	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);
>  }

[Severity: Low]
Does this code preserve the original channel-reported SRB length in the
trace record as stated in the commit message?

The commit message and comments state the original SRB length should be
preserved, but this passes the value-shifted length instead of the original
srb->length to zfcp_dbf_san().

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

[PATCH v5] 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 payload access against the payload offset.
If the reported buffer length does not reach the payload area, suppress
payload reads and avoid constructing a scatterlist for non-existent
payload data.
 
For HBA tracing, preserve serviceability by unconditionally storing the
value-shifted SRB length in pl_len. This retains a reversible mapping to
the original channel-reported length without changing the existing trace
record format, while payload data is only read when the reported buffer
length reaches the payload area.

For SAN tracing, preserve the original channel-reported SRB length in
the trace record and separately track the number of payload bytes that
may be safely read. This retains trace evidence of anomalous
firmware-reported lengths while preventing payload reads 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 v5:
 - Address Steffen's review comments regarding HBA/SAN trace servicability.
 - Preserve HBA pl_len semantics so trace readers can always assume the
   value-shifted subtraction.
 - Retain the original channel-reported SRB length in SAN trace records.
 - Use a separate capped length to control payload reads in the SAN path.
 - Prevent payload reads when the reported buffer length does not reach
   the payload area.
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 capped payload
   length using length/cap_len semantics.
 - Set cap_length to zero when no payload area exists to prevent
   payload reads beyond the reported buffer.
 - 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-v5
---
 drivers/s390/scsi/zfcp_dbf.c | 36 ++++++++++++++++++++++++++----------
 drivers/s390/scsi/zfcp_dbf.h |  2 +-
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..fb55d58e363b 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;
@@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	rec->u.uss.s_id = ntoh24(srb->s_id);
 	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);
+	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
+	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
+	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
+	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
+	 */
+	rec->pl_len = (u16)(srb->length - pay_offset);
 
-	if (rec->pl_len)
+	/* Only access payload bytes when srb->length actually covers them. */
+	if (srb->length > pay_offset)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
 				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
 log:
@@ -714,17 +719,28 @@ 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);
+	/*
+	 * Store the original channel-reported SRB length in pl_len.
+	 * cap_length specifies the number of payload bytes that may be read.
+	 */
+	length = (u16)srb->length;
+	cap_length = 0;
+	if (srb->length > pay_offset) {
+		/* Payload present beyond the fixed header. */
+		cap_length = (u16)(srb->length - pay_offset);
+		sg_init_one(&sg, srb->payload.data, cap_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);
 }
 
 /**
diff --git a/drivers/s390/scsi/zfcp_dbf.h b/drivers/s390/scsi/zfcp_dbf.h
index 79973fb24b1c..81b4d5b79efd 100644
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -310,7 +310,7 @@ struct zfcp_dbf_pay {
 	u8 counter;
 	char area[ZFCP_DBF_TAG_LEN];
 	u64 fsf_req_id;
-#define ZFCP_DBF_PAY_MAX_REC 0x100
+#define ZFCP_DBF_PAY_MAX_REC	4032
 	char data[ZFCP_DBF_PAY_MAX_REC];
 } __packed;
 
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] scsi: zfcp: redundant trace records generated due to srb->length semantics change
- [Low] scsi: zfcp: 32-to-16 bit truncation breaks trace ambiguity goal for large anomalous lengths
--

commit 461d7fe95c4b53bf3dfb0dc33b64ab47c7c0b0f4
Author: Ajaykumar Rajappa <ajaykr@linux.ibm.com>

    zfcp: Fix integer underflow in status read buffer payload length

This patch fixes an integer underflow when calculating the payload
length of a status read buffer in zfcp_dbf_hba_fsf_uss() and
zfcp_dbf_san_in_els(). It validates payload access against the
payload offset before reading payload bytes, while keeping the
serviceability trace records intact.

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

[ ... ]

> @@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
>  	rec->u.uss.s_id = ntoh24(srb->s_id);
>  	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);
> +	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
> +	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
> +	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
> +	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
> +	 */
> +	rec->pl_len = (u16)(srb->length - pay_offset);

[Severity: Low]
Could this 16-bit truncation break the trace ambiguity goal for large
anomalous lengths?

The inline comment claims this creates a "bijective map" that makes anomalous
lengths unambiguously distinguishable from a genuine empty payload. However,
srb->length is a 32-bit field, whereas rec->pl_len is a 16-bit field.

If the hardware provides an anomalously large length (for example,
srb->length == 65536 + pay_offset), the subtraction yields 65536, which
truncates to exactly 0 when cast to u16.

Would this map the anomalous length to exactly 0, making it entirely
indistinguishable from a genuine empty payload and mathematically violating
the stated tracing design goal?

[ ... ]

> @@ -714,17 +719,28 @@ 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);
> +	/*
> +	 * Store the original channel-reported SRB length in pl_len.
> +	 * cap_length specifies the number of payload bytes that may be read.
> +	 */
> +	length = (u16)srb->length;
> +	cap_length = 0;
> +	if (srb->length > pay_offset) {
> +		/* Payload present beyond the fixed header. */
> +		cap_length = (u16)(srb->length - pay_offset);
> +		sg_init_one(&sg, srb->payload.data, cap_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);

[Severity: Low]
Does passing the full srb->length to zfcp_dbf_san() break the length
optimization check inside the trace function?

Inside zfcp_dbf_san(), this length argument is compared against a maximum
inline payload size (ZFCP_DBF_SAN_MAX_PAYLOAD, 48 bytes) to optimize tracing
by determining if a separate payload trace record can be skipped:

    if (len <= rec_len)
        goto out;

Because the full srb->length includes the 60-byte fixed header (pay_offset),
length will always be at least 60 for any valid incoming ELS request.

Consequently, the check length <= rec_len will always evaluate to false.

Can this lead to unconditional and redundant generation of payload trace
records for all payloads, needlessly duplicating data and wasting trace
buffer space?

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

[PATCH v5] 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 payload access against the payload offset.
If the reported buffer length does not reach the payload area, suppress
payload reads and avoid constructing a scatterlist for non-existent
payload data.
 
For HBA tracing, preserve serviceability by unconditionally storing the
value-shifted SRB length in pl_len. This retains a reversible mapping to
the original channel-reported length without changing the existing trace
record format, while payload data is only read when the reported buffer
length reaches the payload area.

For SAN tracing, preserve the original channel-reported SRB length in
the trace record and separately track the number of payload bytes that
may be safely read. This retains trace evidence of anomalous
firmware-reported lengths while preventing payload reads 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 v5:
 - Address Steffen's review comments regarding HBA/SAN trace servicability.
 - Preserve HBA pl_len semantics so trace readers can always assume the
   value-shifted subtraction.
 - Retain the original channel-reported SRB length in SAN trace records.
 - Use a separate capped length to control payload reads in the SAN path.
 - Prevent payload reads when the reported buffer length does not reach
   the payload area.
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 capped payload
   length using length/cap_len semantics.
 - Set cap_length to zero when no payload area exists to prevent
   payload reads beyond the reported buffer.
 - 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-v5
---
 drivers/s390/scsi/zfcp_dbf.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..fb55d58e363b 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;
@@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	rec->u.uss.s_id = ntoh24(srb->s_id);
 	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);
+	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
+	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
+	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
+	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
+	 */
+	rec->pl_len = (u16)(srb->length - pay_offset);
 
-	if (rec->pl_len)
+	/* Only access payload bytes when srb->length actually covers them. */
+	if (srb->length > pay_offset)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
 				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
 log:
@@ -714,17 +719,28 @@ 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);
+	/*
+	 * Store the original channel-reported SRB length in pl_len.
+	 * cap_length specifies the number of payload bytes that may be read.
+	 */
+	length = (u16)srb->length;
+	cap_length = 0;
+	if (srb->length > pay_offset) {
+		/* Payload present beyond the fixed header. */
+		cap_length = (u16)(srb->length - pay_offset);
+		sg_init_one(&sg, srb->payload.data, cap_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 v5] zfcp: Fix integer underflow in status read buffer payload length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] zfcp: missing upper-bound check on srb->length causes out-of-bounds read in tracing
- [Medium] zfcp: trace skipping optimization in zfcp_dbf_san() is broken by full SRB length
--

commit 20c6d5c42ce9fd226b707fb82b26a43e90bfdc4b
Author: Ajaykumar Rajappa <ajaykr@linux.ibm.com>

zfcp: Fix integer underflow in status read buffer payload length

This commit adds checks to prevent integer underflow when calculating
the payload length of a status read buffer (SRB) by verifying that
srb->length is greater than the payload offset. It also modifies
tracing logic for both HBA and SAN paths to conditionally access the
payload only when it exists, while preserving length reporting.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index 81fb8af408e9c..fb55d58e363b9 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
[ ... ]
> @@ -253,11 +254,15 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
>  	rec->u.uss.s_id = ntoh24(srb->s_id);
>  	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);
> +	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
> +	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
> +	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
> +	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
> +	 */
> +	rec->pl_len = (u16)(srb->length - pay_offset);
>  
> -	if (rec->pl_len)
> +	/* Only access payload bytes when srb->length actually covers them. */
> +	if (srb->length > pay_offset)
> +		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
> +				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);

[Severity: High]
Does this code allow an out-of-bounds read if the hardware or firmware returns
an unusually large srb->length?

While the lower bound underflow is now checked, the srb->length field is an
untrusted 32-bit value populated by hardware/firmware via DMA into a buffer
allocated as a single PAGE_SIZE (4096 bytes) element.

If srb->length is a large value (e.g., 0xFFFF), rec->pl_len will be very
large.

Since there is no upper-bound check against FSF_STATUS_READ_PAYLOAD_SIZE,
could zfcp_dbf_pl_write() read thousands of bytes beyond the allocated
page boundary?

[ ... ]
> @@ -714,17 +719,28 @@ 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);
> +	/*
> +	 * Store the original channel-reported SRB length in pl_len.
> +	 * cap_length specifies the number of payload bytes that may be read.
> +	 */
> +	length = (u16)srb->length;
> +	cap_length = 0;
> +	if (srb->length > pay_offset) {
> +		/* Payload present beyond the fixed header. */
> +		cap_length = (u16)(srb->length - pay_offset);
> +		sg_init_one(&sg, srb->payload.data, cap_length);

[Severity: High]
Could an unvalidated large srb->length result in an oversized cap_length here
as well?

If a large value is returned, sg_init_one() will initialize a scatterlist
length that extends well beyond the page boundary, causing zfcp_dbf_san() to
read out-of-bounds memory.

> +	}
> +
> +	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);

[Severity: Medium]
Will this change break the trace skipping optimization in zfcp_dbf_san()?

By passing the full srb->length (including the header) as the len parameter
(the 6th argument) to zfcp_dbf_san(), the len value will always be at least
64 bytes for any valid payload.

Inside zfcp_dbf_san(), the optimization checks:

    if (len <= rec_len)
        goto out; /* skip pay record if full content in rec->payload */

Since rec_len is capped at ZFCP_DBF_SAN_MAX_PAYLOAD (48 bytes), this
condition will always evaluate to false, causing the driver to unconditionally
duplicate small payloads into the secondary pay_buf trace ring.

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

[PATCH v5] 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 payload access against the payload offset.
If the reported buffer length does not reach the payload area, suppress
payload reads and avoid constructing a scatterlist for non-existent
payload data.
 
For HBA tracing, preserve serviceability by unconditionally storing the
value-shifted SRB length in pl_len. This retains a reversible mapping to
the original channel-reported length without changing the existing trace
record format, while payload data is only read when the reported buffer
length reaches the payload area.

For SAN tracing, preserve the original channel-reported SRB length in
the trace record and separately track the number of payload bytes that
may be safely read. This retains trace evidence of anomalous
firmware-reported lengths while preventing payload reads 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 v5:
 - Address Steffen's review comments regarding HBA/SAN trace servicability.
 - Preserve HBA pl_len semantics so trace readers can always assume the
   value-shifted subtraction.
 - Retain the original channel-reported SRB length in SAN trace records.
 - Use a separate capped length to control payload reads in the SAN path.
 - Prevent payload reads when the reported buffer length does not reach
   the payload area.
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-v5
---
 drivers/s390/scsi/zfcp_dbf.c | 38 +++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..c5501eae9977 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;
@@ -253,13 +254,17 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	rec->u.uss.s_id = ntoh24(srb->s_id);
 	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);
+	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
+	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
+	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
+	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
+	 */
+	rec->pl_len = (u16)(srb->length - pay_offset);
 
-	if (rec->pl_len)
+	/* Only access payload bytes when srb->length actually covers them. */
+	if (srb->length > pay_offset)
 		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
-				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
+				  "fsf_uss", req->req_id);
 log:
 	debug_event(dbf->hba, level, rec, sizeof(*rec));
 	spin_unlock_irqrestore(&dbf->hba_lock, flags);
@@ -714,17 +719,28 @@ 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);
+	/*
+	 * Store the original channel-reported SRB length in pl_len.
+	 * cap_length specifies the number of payload bytes that may be read.
+	 */
+	length = (u16)srb->length;
+	cap_length = 0;
+	if (srb->length > pay_offset) {
+		/* Payload present beyond the fixed header. */
+		cap_length = (u16)(srb->length - pay_offset);
+		sg_init_one(&sg, srb->payload.data, cap_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 v5] 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] zfcp: Build failure due to missing 'level' argument in zfcp_dbf_pl_write() call
--

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

zfcp: Fix integer underflow in status read buffer payload length

This commit fixes an integer underflow in status read buffer length
calculations by validating the payload access against the payload offset.
It prevents payload reads when the buffer length does not reach the payload
area, while maintaining existing tracing formatting.

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

[ ... ]

> @@ -253,13 +254,17 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
>  	rec->u.uss.s_id = ntoh24(srb->s_id);
>  	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);
> +	/* Unconditionally store value-shifted SRB length: pl_len is a bijective
> +	 * map of srb->length (pl_len + pay_offset == srb->length). A bogus
> +	 * srb->length < pay_offset wraps to a large non-zero pl_len, which is
> +	 * unambiguously distinguishable from pl_len == 0(genuine empty payload)
> +	 */
> +	rec->pl_len = (u16)(srb->length - pay_offset);
>  
> -	if (rec->pl_len)
> +	/* Only access payload bytes when srb->length actually covers them. */
> +	if (srb->length > pay_offset)
>  		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
> -				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
> +				  "fsf_uss", req->req_id);

[Severity: Low]
Does this change break the build? The function zfcp_dbf_pl_write() is 
defined as requiring 6 arguments:

static inline
void zfcp_dbf_pl_write(struct zfcp_dbf *dbf, void *data, u16 length,
                       char *area, u64 req_id, int level)

However, the modified call above drops the 6th argument 
(ZFCP_DBF_PAY_LEVEL), which will result in a 'too few arguments to function' 
compilation error.

>  log:
>  	debug_event(dbf->hba, level, rec, sizeof(*rec));
>  	spin_unlock_irqrestore(&dbf->hba_lock, flags);

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