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

[sashiko-bot@kernel.org]

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