Re: [PATCH v7 01/19] s390/dasd: Do not complete a failed ESE read as successful

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

Pre-existing issues:
- [Critical] When `dasd=fixedbuffers` is enabled, `dasd_eckd_free_cp()` overwrites the zeroed bio buffer with uninitialized kernel memory from the bounce buffer.
- [High] Missing Timer Cleanup & BH Schedule on early error paths in dasd_int_handler stalls the device queue.
- [High] Calling `rq_data_dir(req)` on `callback_data` causes a kernel panic for internal driver requests because their `callback_data` is a special tag (`DASD_SLEEPON_START_TAG`), not a `struct request *`.
--

--- Patch [1]: [PATCH v7 01/19] s390/dasd: Do not complete a failed ESE read as successful ---
commit 0082fb828aa2cab07732e010fb549548d0fd5d72
Author: Stefan Haberland <[email protected]>

s390/dasd: Do not complete a failed ESE read as successful
    
dasd_int_handler() completes an NRF read of an unallocated ESE track by
calling ese_read() and unconditionally marking the request
DASD_CQR_SUCCESS. dasd_eckd_ese_read() can return an error before it has
zeroed the destination buffer: a failed sense-data parse or a current
track outside the requested range both return early, leaving the
destination pages untouched. The request is still completed successfully,
so the block layer is handed stale / uninitialized memory instead of
zeros.
    
Check the ese_read() return value and fail the request through the normal
error path instead of forcing DASD_CQR_SUCCESS.
    
Fixes: 5e6bdd37c552 ("s390/dasd: fix data corruption for thin provisioned devices")
Cc: [email protected]

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3181c06d91ce3..dbe3caa1e0b4a 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1691,15 +1691,17 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
>  	if (dasd_ese_needs_format(cqr->block, irb)) {
>  		req = dasd_get_callback_data(cqr);
>  		if (!req) {
>  			cqr->status = DASD_CQR_ERROR;
>  			return;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but this early return path seems to
miss cleaning up the timer and scheduling the bottom half.

If !req is true, the request is marked as DASD_CQR_ERROR and returned
early without clearing the timer and scheduling the bottom half via
dasd_device_clear_timer() and dasd_schedule_device_bh() in dasd_int_handler.
Does this leave the request stuck in the queue and stall the block device
queue until the timeout triggers ERP?

>  		if (rq_data_dir(req) == READ) {

[Severity: High]
This is a pre-existing issue, but can this dereference an invalid address?

If an internal driver request (like space release via
dasd_eckd_release_space_trks) is initiated using
dasd_sleep_on_queue_interruptible(), it sets cqr->callback_data to a
special tag (DASD_SLEEPON_START_TAG, which is (void *)1), not a valid
pointer.

Since dasd_get_callback_data() would return 1, the !req check above is
bypassed. Would rq_data_dir(req) then dereference (void *)1 in
dasd_int_handler, causing a kernel panic?

> -			device->discipline->ese_read(cqr, irb);
> -			cqr->status = DASD_CQR_SUCCESS;
> +			if (device->discipline->ese_read(cqr, irb))
> +				cqr->status = DASD_CQR_ERROR;
> +			else
> +				cqr->status = DASD_CQR_SUCCESS;

[Severity: Critical]
This is a pre-existing issue, but is there a potential information leak when
dasd=fixedbuffers is enabled?

If a user reads an unallocated area of a DASD device with dasd=fixedbuffers
enabled, the driver allocates bounce buffers using kmem_cache_alloc without
__GFP_ZERO.

When dasd_eckd_ese_read() successfully zeroes the original bio buffer, this
request completes with DASD_CQR_SUCCESS. During cleanup, does
dasd_eckd_free_cp() unconditionally copy the uninitialized bounce buffer
back over the zeroed bio buffer for READ requests?

drivers/s390/block/dasd_eckd.c:dasd_eckd_free_cp() {
    ...
				if (dst != cda) {
					if (rq_data_dir(req) == READ)
						memcpy(dst, cda, bv.bv_len);
					kmem_cache_free(dasd_page_cache,
					    (void *)((addr_t)cda & PAGE_MASK));
				}
    ...
}

Could this overwrite the zeros populated by ese_read() with uninitialized
kernel memory, which is then handed to userspace?

>  			cqr->stopclk = now;
>  			dasd_device_clear_timer(device);
>  			dasd_schedule_device_bh(device);

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