Re: [PATCH v13 2/2] zfcp: Use DMA streaming API to map I/O

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] scsi: zfcp: scsi_cmnd UAF on abort failure in zfcp_scsi_eh_abort_handler()
- [High] scsi: zfcp: DMA mapping leak in zfcp_scsi_eh_abort_handler()
- [Medium] zfcp: 31-bit coherent_dma_mask overwritten in zfcp_set_dma_mask_for_adapter()
--

commit ee42d1a728b67714b2c6be28df7f409c25bc9e1f
Author: M Nikhil <nikh1092@linux.ibm.com>

zfcp: Use DMA streaming API to map I/O

This commit converts zFCP I/O buffer mappings (QTCB, SRB, CT/ELS, and SCSI
I/O) from direct physical address translations to the DMA streaming API using
dma_map_sgtable() and dma_unmap_sgtable().

> diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
> index fea573e00f9df..20c905c68e1e6 100644
> --- a/drivers/s390/scsi/zfcp_aux.c
> +++ b/drivers/s390/scsi/zfcp_aux.c
> @@ -329,6 +329,19 @@ static void zfcp_destroy_adapter_work_queue(struct zfcp_adapter *adapter)
>  
>  }
>  
> +static int zfcp_set_dma_mask_for_adapter(struct zfcp_adapter *adapter)
> +{
> +	int ret;
> +
> +	ret = dma_set_mask_and_coherent(&adapter->ccw_device->dev,
> +					DMA_BIT_MASK(64));
> +	if (ret)
> +		dev_err(&adapter->ccw_device->dev,
> +			"Setting the DMA mask failed for the FCP device.\n");
> +
> +	return ret;
> +}

[Severity: Medium]
Should zfcp_set_dma_mask_for_adapter() call dma_set_mask() instead of
dma_set_mask_and_coherent()?

In drivers/s390/cio/css.c:css_alloc_subchannel() and
drivers/s390/cio/device.c:io_subchannel_allocate_dev(), the s390 CIO core
initializes cdev->dev.dma_mask to DMA_BIT_MASK(64) for streaming DMA while
explicitly keeping cdev->dev.coherent_dma_mask at DMA_BIT_MASK(31):

drivers/s390/cio/css.c:css_alloc_subchannel() {
	/*
	 * The physical addresses for some of the dma structures that can
	 * belong to a subchannel need to fit 31 bit width (e.g. ccw).
	 */
	ret = dma_set_coherent_mask(&sch->dev, DMA_BIT_MASK(31));
	if (ret)
		goto err;
	/*
	 * But we don't have such restrictions imposed on the stuff that
	 * is handled by the streaming API.
	 */
	ret = dma_set_mask(&sch->dev, DMA_BIT_MASK(64));
}

Calling dma_set_mask_and_coherent() overwrites cdev->dev.coherent_dma_mask
with a 64-bit mask even though zFCP only uses streaming DMA. If an online
zFCP device is detached under z/VM or KVM, zfcp_ccw_notify() returns
NOTIFY_OK on CIO_GONE and io_subchannel_sch_event() moves the struct
ccw_device to the pseudo-subchannel orphanage via ccw_device_move_to_orph().
When another CCW device (such as virtio-ccw) is later attached at the same
device number and rebound via ccw_device_recog_done() ->
ccw_device_do_unbind_bind(), coherent allocations via
ccw_device_dma_zalloc() -> __cio_gp_dma_zalloc() will use the clobbered
64-bit coherent_dma_mask, allowing dma_alloc_coherent() to return memory
above 2 GiB and truncate dma_addr to 31-bit dma32_t.

[ ... ]

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index ea4b968352f4d..32bfb7e4891ee 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -179,11 +179,20 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  	write_lock_irqsave(&adapter->abort_lock, flags);
>  
>  	old_req = zfcp_reqlist_find(adapter->req_list, old_reqid);
> -	if (!old_req) {
> +	if (!old_req || !old_req->data) {
>  		write_unlock_irqrestore(&adapter->abort_lock, flags);
>  		zfcp_dbf_scsi_abort("abrt_or", scpnt, NULL);
>  		return FAILED; /* completion could be in progress */
>  	}
> +
> +	/*
> +	 * Suppress the normal completion handler's unmap by clearing
> +	 * req->data now, under the abort_lock write-side.  The actual
> +	 * DMA unmap is deferred until the hardware has stopped using
> +	 * the buffers (after wait_for_completion or after ERP has
> +	 * quiesced the adapter), so that we never call dma_unmap_sgtable
> +	 * while the FCP channel may still be writing into the pages.
> +	 */
>  	old_req->data = NULL;
>  
>  	/* don't access old fsf_req after releasing the abort_lock */
> @@ -198,18 +207,34 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  		zfcp_erp_wait(adapter);
>  		ret = fc_block_scsi_eh(scpnt);
>  		if (ret) {
> +			/*
> +			 * fc_block_scsi_eh returned FAST_IO_FAIL: the
> +			 * fast_io_fail_tmo fired.  ERP has already called
> +			 * zfcp_fsf_req_dismiss_all() (which requires QDIO
> +			 * to be down first), so the hardware is no longer
> +			 * using the buffers.  Pass FAST_IO_FAIL back to
> +			 * scsi_eh, but unmap first.
> +			 */
>  			zfcp_dbf_scsi_abort("abrt_bl", scpnt, NULL);
> -			return ret;
> +			retval = ret;
> +			goto out_unmap;
>  		}
>  		if (!(atomic_read(&adapter->status) &
>  		      ZFCP_STATUS_COMMON_RUNNING)) {
> +			/*
> +			 * Adapter is gone; ERP shut down QDIO and dismissed
> +			 * all requests before clearing RUNNING.  Safe to
> +			 * unmap now.
> +			 */
>  			zfcp_dbf_scsi_abort("abrt_ru", scpnt, NULL);
> -			return SUCCESS;
> +			retval = SUCCESS;
> +			goto out_unmap;
>  		}
>  	}
>  	if (!abrt_req) {
>  		zfcp_dbf_scsi_abort("abrt_ar", scpnt, NULL);
> -		return FAILED;
> +		retval = FAILED;
> +		goto out_unmap;
>  	}
>  
>  	wait_for_completion(&abrt_req->completion);
> @@ -224,6 +249,39 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  	}
>  	zfcp_dbf_scsi_abort(dbf_tag, scpnt, abrt_req);
>  	zfcp_fsf_req_free(abrt_req);
> +
> +out_unmap:
> +	if (retval == SUCCESS) {
> +		/*
> +		 * The hardware has stopped using the SCSI command's DMA buffers
> +		 * either the adapter confirmed the abort (abrt_ok / abrt_nn), or
> +		 * ERP has shutdown QDIO and cleared RUNNING (abrt_ru).
> +		 */
> +		if ((scsi_get_prot_op(scpnt) != SCSI_PROT_NORMAL) &&
> +		    scsi_prot_sg_count(scpnt))
> +			zfcp_fsf_req_unmap_sg(&adapter->ccw_device->dev,
> +					      &scpnt->prot_sdb->table,
> +					      scpnt->sc_data_direction);
> +
> +		zfcp_fsf_req_unmap_sg(&adapter->ccw_device->dev,
> +				      &scpnt->sdb.table,
> +				      scpnt->sc_data_direction);
> +	} else {
> +		/*
> +		 * Abort failed and adapter was not quiesced (abrt_ar, abrt_fa,
> +		 * or abrt_bl). Do not unmap DMA buffers while hardware may still
> +		 * be accessing them. Restore old_req->data so that subsequent
> +		 * SCSI EH reset escalation (zfcp_scsi_forget_cmnd) or ERP
> +		 * dismissal (zfcp_fsf_req_dismiss_all) unmaps the buffers
> +		 * once the hardware has actually stopped.
> +		 */
> +		write_lock_irqsave(&adapter->abort_lock, flags);
> +		old_req = zfcp_reqlist_find(adapter->req_list, old_reqid);
> +		if (old_req)
> +			old_req->data = scpnt;
> +		write_unlock_irqrestore(&adapter->abort_lock, flags);
> +	}
> +
>  	return retval;
>  }

[Severity: High]
Does out_unmap in zfcp_scsi_eh_abort_handler() leak the DMA mappings for
scpnt->sdb.table and scpnt->prot_sdb->table when retval != SUCCESS and
old_req was already removed from adapter->req_list?

After zfcp_scsi_eh_abort_handler() sets old_req->data = NULL and drops
adapter->abort_lock, old_req can complete or be dismissed by ERP via
zfcp_qdio_close() -> zfcp_fsf_req_dismiss_all() -> zfcp_fsf_req_complete()
while the abort handler is in its retry loop or waiting on
abrt_req->completion. Because req->data is NULL, zfcp_fsf_fcp_cmnd_handler()
returns early without unmapping scpnt's scatterlists, and old_req is removed
from adapter->req_list and freed:

drivers/s390/scsi/zfcp_fsf.c:zfcp_fsf_fcp_cmnd_handler() {
	scpnt = req->data;
	if (unlikely(!scpnt)) {
		read_unlock_irqrestore(&req->adapter->abort_lock, flags);
		return;
	}
}

If zfcp_scsi_eh_abort_handler() then reaches out_unmap with retval != SUCCESS
-- for example, when fc_block_scsi_eh() returns FAST_IO_FAIL ("abrt_bl",
where the comment notes it should unmap first, but retval is FAST_IO_FAIL so
the retval == SUCCESS branch is skipped), when all retries of
zfcp_fsf_abort_fcp_cmnd() return NULL ("abrt_ar"), or when abrt_req itself is
dismissed by ERP ("abrt_fa") -- execution enters the else branch. There,
zfcp_reqlist_find(adapter->req_list, old_reqid) returns NULL because old_req
is already gone, so neither out_unmap nor any later zfcp_scsi_forget_cmnd()
or zfcp_fsf_req_dismiss_all() will ever unmap scpnt->sdb.table or
scpnt->prot_sdb->table.

[Severity: Critical]
In the case where old_req is still in adapter->req_list when out_unmap enters
the else branch, can restoring old_req->data = scpnt cause a use-after-free
on scpnt in zfcp_fsf_fcp_cmnd_handler()?

Prior to this change, zfcp_scsi_eh_abort_handler() permanently cleared
old_req->data = NULL under adapter->abort_lock so that
zfcp_fsf_fcp_cmnd_handler() could never touch scpnt or call scsi_done(scpnt)
once abort handling started.

With old_req->data = scpnt restored on retval != SUCCESS:

1. If FAST_IO_FAIL is returned while old_req is still in adapter->req_list --
   either from fc_block_scsi_eh(scpnt) in zfcp_scsi_eh_abort_handler() (for
   example, when a single remote port's fast_io_fail_tmo fires via
   fc_timeout_fail_rport_io() -> zfcp_scsi_terminate_rport_io() ->
   zfcp_erp_port_forced_reopen(), which closes the port asynchronously
   without shutting down QDIO or calling zfcp_fsf_req_dismiss_all()), or
   during subsequent EH escalation in zfcp_scsi_task_mgmt_function() when
   fc_block_rport(rport) returns FAST_IO_FAIL before
   zfcp_scsi_forget_cmnds() is reached:

   drivers/s390/scsi/zfcp_scsi.c:zfcp_scsi_task_mgmt_function() {
		ret = fc_block_rport(rport);
		if (ret) {
			zfcp_dbf_scsi_devreset("fiof", sdev, tm_flags, NULL);
			return ret;
		}
   }

   -- SCSI EH immediately finishes scpnt via scsi_eh_finish_cmd() ->
   scsi_finish_command() -> scsi_free_sgtables(). That frees scpnt's
   scatter-gather tables while they are still DMA-mapped and releases scpnt
   for reuse while old_req->data still points to scpnt. When old_req later
   completes or is dismissed, zfcp_fsf_fcp_cmnd_handler() will dereference
   the freed or reused scpnt, unmap its freed sdb.table and prot_sdb->table,
   and call scsi_done(scpnt).

2. Even when FAST_IO_FAIL is not returned, restoring old_req->data = scpnt
   allows any late completion or ERP dismissal of old_req during SCSI EH
   escalation to execute zfcp_fsf_fcp_cmnd_handler(), modifying scpnt and
   calling scsi_done(scpnt) concurrently with SCSI EH.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023114046.4137557-1-nikh1092@linux.ibm.com?part=2