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:
- [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