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

[PATCH v6] 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.

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>
---
 drivers/s390/scsi/zfcp_dbf.c | 103 +++++++++++++++++++++++++++++++----
 1 file changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..a71dc0696d80 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -65,6 +65,47 @@ void zfcp_dbf_pl_write(struct zfcp_dbf *dbf, void *data, u16 length, char *area,
 	spin_unlock(&dbf->pay_lock);
 }
 
+/**
+ * zfcp_dbf_srb_pl_len - payload length to be traced for a status read buffer
+ * @srb: status read buffer as received from the channel
+ *
+ * A length of 0 (buffer not filled, e.g. dismissed request) means no payload.
+ * Otherwise the payload length is srb->length without the header. A bogus
+ * srb->length below the header size wraps to a large value and is thereby
+ * distinguishable from a genuine empty payload.
+ */
+static inline u16 zfcp_dbf_srb_pl_len(struct fsf_status_read_buffer *srb)
+{
+	if (!srb->length)
+		return 0;
+	return (u16)(srb->length -
+		     offsetof(struct fsf_status_read_buffer, payload));
+}
+
+/**
+ * zfcp_dbf_srb_len_trace_if_bogus - trace raw SRB length if out of range
+ * @dbf: debug feature of the adapter
+ * @srb: status read buffer as received from the channel
+ * @req_id: request ID used to correlate with the HBA and SAN trace records
+ *
+ * The u16 pl_len in the trace records cannot hold the full 32-bit
+ * srb->length. Unless the length is 0 or within the header plus payload
+ * array size, write the raw 32-bit value as a separate "fsf_len" pay record.
+ */
+static void zfcp_dbf_srb_len_trace_if_bogus(struct zfcp_dbf *dbf,
+					    struct fsf_status_read_buffer *srb,
+					    u64 req_id)
+{
+	u32 raw_len = srb->length;
+
+	if (!raw_len ||
+	    (raw_len >= offsetof(struct fsf_status_read_buffer, payload) &&
+	     raw_len <= sizeof(*srb)))
+		return;
+
+	zfcp_dbf_pl_write(dbf, &raw_len, sizeof(raw_len), "fsf_len", req_id);
+}
+
 /**
  * zfcp_dbf_hba_fsf_res - trace event for fsf responses
  * @tag: tag indicating which kind of FSF response has been received
@@ -223,6 +264,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 +295,30 @@ 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);
+	/* pl_len is the payload length (srb->length - pay_offset) as long as
+	 * srb->length is valid. For a bogus srb->length it is only derived
+	 * from it and the raw length is traced as "fsf_len" below.
+	 */
+	rec->pl_len = zfcp_dbf_srb_pl_len(srb);
+
+	/* Only access payload bytes when srb->length actually covers them.
+	 * Bounds are checked on the full 32-bit value (before any u16
+	 * truncation). An oversized srb->length is not dropped: the payload
+	 * is traced up to the end of the payload array (diagnostic value).
+	 */
+	if (srb->length > pay_offset) {
+		u32 cap_len = srb->length - pay_offset;
 
-	if (rec->pl_len)
-		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
-				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
+		if (cap_len > FSF_STATUS_READ_PAYLOAD_SIZE)
+			cap_len = FSF_STATUS_READ_PAYLOAD_SIZE;
+		zfcp_dbf_pl_write(dbf, srb->payload.data, cap_len,
+				  "fsf_uss", req->req_id);
+	}
+	/* pl_len is only 16 bit: keep the raw length for out-of-range cases.
+	 * Done only here as this runs for every status read buffer, including
+	 * incoming ELS which is traced again by zfcp_dbf_san_in_els().
+	 */
+	zfcp_dbf_srb_len_trace_if_bogus(dbf, srb, req->req_id);
 log:
 	debug_event(dbf->hba, level, rec, sizeof(*rec));
 	spin_unlock_irqrestore(&dbf->hba_lock, flags);
@@ -714,17 +773,37 @@ 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 pl_len, cap_length = 0;
 	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);
+	/* pl_len is the payload length as reported by the channel (same
+	 * mapping as in zfcp_dbf_hba_fsf_uss()). It is passed as "len" so
+	 * that zfcp_dbf_san() can skip the separate pay record when the
+	 * payload fits into rec->payload. The raw length of a bogus
+	 * srb->length is traced as "fsf_len" by zfcp_dbf_hba_fsf_uss().
+	 *
+	 * cap_length is the number of payload bytes that may actually be
+	 * read: bounds are checked on the full 32-bit srb->length and an
+	 * oversized value is clamped to the payload array size, so the
+	 * readable part is still traced.
+	 */
+	pl_len = zfcp_dbf_srb_pl_len(srb);
+	if (srb->length > pay_offset) {
+		u32 cap = srb->length - pay_offset;
+
+		if (cap > FSF_STATUS_READ_PAYLOAD_SIZE)
+			cap = FSF_STATUS_READ_PAYLOAD_SIZE;
+		cap_length = cap;
+		sg_init_one(&sg, srb->payload.data, cap_length);
+	}
+
+	zfcp_dbf_san(tag, dbf, "san_els", cap_length ? &sg : NULL,
+		     ZFCP_DBF_SAN_ELS, pl_len, fsf->req_id,
+		     ntoh24(srb->d_id), cap_length);
 }
 
 /**
-- 
2.53.0

Re: [PATCH v6] 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 missing argument in zfcp_dbf_pl_write()
--

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

zfcp: Fix integer underflow in status read buffer payload length

This commit addresses integer underflows in zfcp_dbf_hba_fsf_uss() and
zfcp_dbf_san_in_els() when srb->length is smaller than the fixed header size.
It adds validations to avoid out-of-bounds reads or constructing invalid
scatterlists.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index 81fb8af408e9c..a71dc0696d809 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
[ ... ]
> @@ -65,6 +65,47 @@ void zfcp_dbf_pl_write(struct zfcp_dbf *dbf, void *data, u16 length, char *area,
>  	spin_unlock(&dbf->pay_lock);
>  }
>  
[ ... ]
> +static void zfcp_dbf_srb_len_trace_if_bogus(struct zfcp_dbf *dbf,
> +					    struct fsf_status_read_buffer *srb,
> +					    u64 req_id)
> +{
> +	u32 raw_len = srb->length;
> +
> +	if (!raw_len ||
> +	    (raw_len >= offsetof(struct fsf_status_read_buffer, payload) &&
> +	     raw_len <= sizeof(*srb)))
> +		return;
> +
> +	zfcp_dbf_pl_write(dbf, &raw_len, sizeof(raw_len), "fsf_len", req_id);

[Severity: Low]
Does this call to zfcp_dbf_pl_write() result in a build regression?

The function zfcp_dbf_pl_write() requires 6 arguments, but only 5 are
provided here. The level argument is missing.

> +}
> +
[ ... ]
> @@ -253,13 +295,30 @@ 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);
> +	/* pl_len is the payload length (srb->length - pay_offset) as long as
> +	 * srb->length is valid. For a bogus srb->length it is only derived
> +	 * from it and the raw length is traced as "fsf_len" below.
> +	 */
> +	rec->pl_len = zfcp_dbf_srb_pl_len(srb);
> +
> +	/* Only access payload bytes when srb->length actually covers them.
> +	 * Bounds are checked on the full 32-bit value (before any u16
> +	 * truncation). An oversized srb->length is not dropped: the payload
> +	 * is traced up to the end of the payload array (diagnostic value).
> +	 */
> +	if (srb->length > pay_offset) {
> +		u32 cap_len = srb->length - pay_offset;
>  
> -	if (rec->pl_len)
> -		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
> -				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
> +		if (cap_len > FSF_STATUS_READ_PAYLOAD_SIZE)
> +			cap_len = FSF_STATUS_READ_PAYLOAD_SIZE;
> +		zfcp_dbf_pl_write(dbf, srb->payload.data, cap_len,
> +				  "fsf_uss", req->req_id);

[Severity: Low]
Could this be missing the level argument (ZFCP_DBF_PAY_LEVEL) as well?

This call to zfcp_dbf_pl_write() drops the 6th argument that was present
in the previous implementation, which also causes a build regression.

> +	}

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

[PATCH v6] 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.

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>
---
 drivers/s390/scsi/zfcp_dbf.c | 104 +++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 81fb8af408e9..b4afb3989363 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -65,6 +65,48 @@ void zfcp_dbf_pl_write(struct zfcp_dbf *dbf, void *data, u16 length, char *area,
 	spin_unlock(&dbf->pay_lock);
 }
 
+/**
+ * zfcp_dbf_srb_pl_len - payload length to be traced for a status read buffer
+ * @srb: status read buffer as received from the channel
+ *
+ * A length of 0 (buffer not filled, e.g. dismissed request) means no payload.
+ * Otherwise the payload length is srb->length without the header. A bogus
+ * srb->length below the header size wraps to a large value and is thereby
+ * distinguishable from a genuine empty payload.
+ */
+static inline u16 zfcp_dbf_srb_pl_len(struct fsf_status_read_buffer *srb)
+{
+	if (!srb->length)
+		return 0;
+	return (u16)(srb->length -
+		     offsetof(struct fsf_status_read_buffer, payload));
+}
+
+/**
+ * zfcp_dbf_srb_len_trace_if_bogus - trace raw SRB length if out of range
+ * @dbf: debug feature of the adapter
+ * @srb: status read buffer as received from the channel
+ * @req_id: request ID used to correlate with the HBA and SAN trace records
+ *
+ * The u16 pl_len in the trace records cannot hold the full 32-bit
+ * srb->length. Unless the length is 0 or within the header plus payload
+ * array size, write the raw 32-bit value as a separate "fsf_len" pay record.
+ */
+static void zfcp_dbf_srb_len_trace_if_bogus(struct zfcp_dbf *dbf,
+					    struct fsf_status_read_buffer *srb,
+					    u64 req_id)
+{
+	u32 raw_len = srb->length;
+
+	if (!raw_len ||
+	    (raw_len >= offsetof(struct fsf_status_read_buffer, payload) &&
+	     raw_len <= sizeof(*srb)))
+		return;
+
+	zfcp_dbf_pl_write(dbf, &raw_len, sizeof(raw_len), "fsf_len", req_id,
+			  ZFCP_DBF_PAY_LEVEL);
+}
+
 /**
  * zfcp_dbf_hba_fsf_res - trace event for fsf responses
  * @tag: tag indicating which kind of FSF response has been received
@@ -223,6 +265,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 +296,30 @@ 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);
+	/* pl_len is the payload length (srb->length - pay_offset) as long as
+	 * srb->length is valid. For a bogus srb->length it is only derived
+	 * from it and the raw length is traced as "fsf_len" below.
+	 */
+	rec->pl_len = zfcp_dbf_srb_pl_len(srb);
+
+	/* Only access payload bytes when srb->length actually covers them.
+	 * Bounds are checked on the full 32-bit value (before any u16
+	 * truncation). An oversized srb->length is not dropped: the payload
+	 * is traced up to the end of the payload array (diagnostic value).
+	 */
+	if (srb->length > pay_offset) {
+		u32 cap_len = srb->length - pay_offset;
 
-	if (rec->pl_len)
-		zfcp_dbf_pl_write(dbf, srb->payload.data, rec->pl_len,
-				  "fsf_uss", req->req_id, ZFCP_DBF_PAY_LEVEL);
+		if (cap_len > FSF_STATUS_READ_PAYLOAD_SIZE)
+			cap_len = FSF_STATUS_READ_PAYLOAD_SIZE;
+		zfcp_dbf_pl_write(dbf, srb->payload.data, cap_len, "fsf_uss",
+				  req->req_id, ZFCP_DBF_PAY_LEVEL);
+	}
+	/* pl_len is only 16 bit: keep the raw length for out-of-range cases.
+	 * Done only here as this runs for every status read buffer, including
+	 * incoming ELS which is traced again by zfcp_dbf_san_in_els().
+	 */
+	zfcp_dbf_srb_len_trace_if_bogus(dbf, srb, req->req_id);
 log:
 	debug_event(dbf->hba, level, rec, sizeof(*rec));
 	spin_unlock_irqrestore(&dbf->hba_lock, flags);
@@ -714,17 +774,37 @@ 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 pl_len, cap_length = 0;
 	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);
+	/* pl_len is the payload length as reported by the channel (same
+	 * mapping as in zfcp_dbf_hba_fsf_uss()). It is passed as "len" so
+	 * that zfcp_dbf_san() can skip the separate pay record when the
+	 * payload fits into rec->payload. The raw length of a bogus
+	 * srb->length is traced as "fsf_len" by zfcp_dbf_hba_fsf_uss().
+	 *
+	 * cap_length is the number of payload bytes that may actually be
+	 * read: bounds are checked on the full 32-bit srb->length and an
+	 * oversized value is clamped to the payload array size, so the
+	 * readable part is still traced.
+	 */
+	pl_len = zfcp_dbf_srb_pl_len(srb);
+	if (srb->length > pay_offset) {
+		u32 cap = srb->length - pay_offset;
+
+		if (cap > FSF_STATUS_READ_PAYLOAD_SIZE)
+			cap = FSF_STATUS_READ_PAYLOAD_SIZE;
+		cap_length = cap;
+		sg_init_one(&sg, srb->payload.data, cap_length);
+	}
+
+	zfcp_dbf_san(tag, dbf, "san_els", cap_length ? &sg : NULL,
+		     ZFCP_DBF_SAN_ELS, pl_len, fsf->req_id,
+		     ntoh24(srb->d_id), cap_length);
 }
 
 /**
-- 
2.53.0

Re: [PATCH v6] 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 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.
> 
> 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/20261013023817.1468967-1-ajaykr@linux.ibm.com?part=1