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

[sashiko-bot@kernel.org]

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