Re: [PATCH v2 1/3] zfcp: Enhance fsf status read buffer tracing

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] An integer underflow or missing upper-bounds check when calculating the payload length from an untrusted hardware response allows a massive out-of-bounds read, potentially leaking sensitive kernel memory into the debug trace ring buffer or causing a kernel panic.
--

--- Patch [1]: [PATCH v2 1/3] zfcp: Enhance fsf status read buffer tracing ---
commit bb1109132aa94648a078f3011317837aeee0a3b8
Author: Chinmaya Kajagar <[email protected]>

zfcp: Enhance fsf status read buffer tracing

This commit enhances FSF status read buffer (SRB) tracing by adding missing
fields to the zfcp_dbf_hba_uss structure. It appends the remaining SRB fields
to the unsolicited status trace records and extends the driver to get the
source ID from a reserved field.

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index 71f625926ae19..89b859176b8bd 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -220,6 +220,13 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
>  	rec->u.uss.lun = srb->fcp_lun;
>  	memcpy(&rec->u.uss.queue_designator, &srb->queue_designator,
>  	       sizeof(rec->u.uss.queue_designator));
> +	rec->u.uss.length = srb->length;
> +	rec->u.uss.res1 = srb->res1;
> +	rec->u.uss.res2 = srb->res2;
> +	rec->u.uss.class = srb->class;
> +	rec->u.uss.res3 = srb->res3;
> +	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);

[Severity: High]
This is a pre-existing issue, but can this length calculation underflow if the
hardware returns an srb->length smaller than the payload offset?

If srb->length is non-zero but smaller than offsetof(struct
fsf_status_read_buffer, payload) (which is around 64 bytes), rec->pl_len
will underflow and result in a massive unsigned integer.

If that happens, zfcp_dbf_pl_write() could read out-of-bounds past the
single-page allocation boundary, potentially leaking adjacent kernel memory
into the debug trace or causing a page fault panic.

Does this same missing bounds check also exist in zfcp_dbf_san_in_els()?

drivers/s390/scsi/zfcp_dbf.c:zfcp_dbf_san_in_els() {
    ...
    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, ...);
    ...
}

It appears this location also subtracts the offset without checking for
underflow or a zero length, meaning sg_init_one() and zfcp_dbf_san() could
process an extremely large underflowed length.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1