Re: [PATCH 04/19] s390/dasd: Snapshot intrc before freeing the request block

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

Pre-existing issues:
- [Critical] Missing `blk_rq_has_data()` / `bio_has_data()` guard in `dasd_fba_free_cp` causes NULL pointer dereference for dataless requests.
- [High] Accessing `req->q` after calling `blk_mq_end_request()` results in a use-after-free.
- [High] Dereferencing `cqr->dq` to release a lock after `cqr` has been freed leads to a use-after-free in `dasd_flush_block_queue()`.
- [High] Block layer request use-after-free in `dasd_generic_requeue_all_requests` error path.
--

--- Patch [4]: [PATCH 04/19] s390/dasd: Snapshot intrc before freeing the request block ---
commit 8cd8a0d1124e90e3f5df17a63495a14c3de26d80
Author: Stefan Haberland <[email protected]>

s390/dasd: Snapshot intrc before freeing the request block
    
This patch fixes a race condition where the request block's intrc field is
read after the block is returned to the memory pool. By snapshotting intrc
before calling free_cp(), it prevents concurrent reallocations from
overwriting the completion status.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -2699,17 +2699,23 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
>  	struct request *req;
>  	blk_status_t error = BLK_STS_OK;
>  	unsigned int proc_bytes;
> -	int status;
> +	int status, intrc;
>  
>  	req = (struct request *) cqr->callback_data;
>  	dasd_profile_end(cqr->block, cqr, req);
>  
> +	/*
> +	 * free_cp() returns the request block to its memory pool, so snapshot
> +	 * everything still needed from cqr before calling it - another CPU can
> +	 * reallocate and overwrite the block right after.
> +	 */
>  	proc_bytes = cqr->proc_bytes;
> +	intrc = cqr->intrc;
>  	status = cqr->block->base->discipline->free_cp(cqr, req);
>  	if (status < 0)
>  		error = errno_to_blk_status(status);
>  	else if (status == 0) {
> -		switch (cqr->intrc) {
> +		switch (intrc) {
>  		case -EPERM:
>  			/*
>  			 * DASD doesn't implement SCSI/NVMe reservations, but it

[Severity: High]
This is a pre-existing issue, but since we are fixing a use-after-free
around free_cp() returning the block to the pool, does
dasd_flush_block_queue() have a similar problem?

drivers/s390/block/dasd.c:dasd_flush_block_queue() {
    ...
    __dasd_cleanup_cqr(cqr);
    spin_unlock_irqrestore(&cqr->dq->lock, flags);
    ...
}

Once __dasd_cleanup_cqr(cqr) completes, free_cp() has been called and
the cqr memory block might be reallocated by another thread. Does the
subsequent access to cqr->dq->lock dereference freed memory?

[Severity: High]
This is also a pre-existing issue, but is there a use-after-free on req->q 
later in __dasd_cleanup_cqr() and dasd_request_done()?

drivers/s390/block/dasd.c:__dasd_cleanup_cqr() {
    ...
    blk_mq_end_request(req, error);
    blk_mq_run_hw_queues(req->q, true);
    ...
}

The blk_mq_end_request() call completes the request, meaning the block
layer can instantly reallocate or poison the memory backing req. Does
accessing req->q on the next line dereference this potentially freed
memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in 
dasd_generic_requeue_all_requests() expose the request to a concurrent 
use-after-free?

drivers/s390/block/dasd.c:dasd_generic_requeue_all_requests() {
    ...
    _dasd_requeue_request(cqr);
    list_del_init(&cqr->blocklist);
    cqr->block->base->discipline->free_cp(cqr, 
            (struct request *) cqr->callback_data);
    ...
}

The _dasd_requeue_request(cqr) call invokes blk_mq_requeue_request(req, true),
handing the request back to the block layer and asynchronously kicking the
hardware queue. Can another thread immediately dispatch and complete the
request before free_cp() iterates over the request's bios?

[Severity: Critical]
This is a pre-existing issue, but can dataless requests cause a null pointer 
dereference in dasd_fba_free_cp()?

drivers/s390/block/dasd_fba.c:dasd_fba_free_cp() {
    ...
    rq_for_each_segment(bv, req, iter) {
        dst = bvec_virt(&bv);
        for (off = 0; off < bv.bv_len; off += blksize) {
    ...
}

Operations like REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES have no data
buffers (bio->bi_io_vec is null) but can have a non-zero bi_size. The
rq_for_each_segment() macro expands to bio_for_each_segment(), which
attempts to fetch the current segment from the null bi_io_vec. Should
this iteration be guarded with a check for blk_rq_has_data(req)?

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