Re: [PATCH v7 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] When the `dasd=fixedbuffers` (`dasd_page_cache`) module parameter is enabled, `dasd_eckd_free_cp()` misinterprets the unconditionally created transport-mode (TPM) ESE format replacement request as a command-mode (CCW) request, causing invalid frees of arbitrary memory.
--

--- Patch [12]: [PATCH v7 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit e3d3248abe2fbc8a98e5dc48c8f9561bcf553195
Author: Stefan Haberland <[email protected]>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 475c0aca4a5ae..e731d7e183121 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -5328,6 +5399,15 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
>  	sector_t recid;
>  	int status;
>  
> +	/*
> +	 * A format-aborted request finished nothing - its replacement
> +	 * completes the block request - so report ABORTED instead of DONE,
> +	 * but still release its bounce buffers like any other request.
> +	 */
> +	if (cqr->status == DASD_CQR_ABORTED)
> +		status = DASD_CQR_ABORTED;
> +	else
> +		status = cqr->status == DASD_CQR_DONE;
>  	if (!dasd_page_cache)
>  		goto out;

[Severity: High]
Can this corrupt memory when the dasd_page_cache module parameter is enabled?

The new ESE format replacement request created by
dasd_eckd_build_cp_tpm_writefulltrack() unconditionally sets cqr->cpmode = 1
(Transport Mode) and uses an ITCW, regardless of dasd_page_cache.

However, the replacement request takes ownership of the block request in
dasd_eckd_ese_format():

    fcqr->callback_data = base_cqr->callback_data;

This causes the replacement request to be routed here to dasd_eckd_free_cp()
upon completion.

If the kernel is booted with dasd=fixedbuffers (which enables
dasd_page_cache), dasd_eckd_free_cp() will proceed past this check since
dasd_page_cache is true.

Because dasd_eckd_free_cp() lacks a check for cqr->cpmode == 0, it appears
this could cause dasd_eckd_free_cp() to iterate over the ITCW memory as if it
were a list of struct ccw1, extract garbage addresses, and pass them to
kmem_cache_free().

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