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 2 potential issue(s) to consider:
- [Critical] scsi: zfcp: premature command finish and UAF on FAST_IO_FAIL in TMF reset
- [High] scsi: zfcp: DMA mapping leak and double map after zfcp_scsi_eh_abort_handler()
--

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

zfcp: Use DMA streaming API to map I/O

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

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index ea4b968352f4d..bda27153c3268 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -179,11 +179,31 @@ 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 is left NULL permanently and never restored.
> +	 * Restoring it while returning non-SUCCESS to scsi_eh would be
> +	 * unsafe: scsi_eh is free to call scsi_finish_command() on scpnt
> +	 * as soon as we return non-SUCCESS, freeing scpnt's scatter-gather
> +	 * tables while old_req (with data pointing at the freed scpnt) is
> +	 * still live.  Any subsequent completion or ERP dismissal of
> +	 * old_req would then dereference a freed scpnt in
> +	 * zfcp_fsf_fcp_cmnd_handler() -- use-after-free and double
> +	 * scsi_done().  Leaving data = NULL means zfcp_fsf_fcp_cmnd_handler
> +	 * returns early harmlessly if old_req completes after we return.
> +	 */
>  	old_req->data = NULL;
>  
>  	/* don't access old fsf_req after releasing the abort_lock */
> @@ -198,16 +218,55 @@ 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 because
> +			 * fast_io_fail_tmo fired.  This may originate from a
> +			 * per-port reopen (fc_timeout_fail_rport_io ->
> +			 * zfcp_scsi_terminate_rport_io ->
> +			 * zfcp_erp_port_forced_reopen) that closes the port
> +			 * asynchronously without shutting QDIO down or
> +			 * calling zfcp_fsf_req_dismiss_all().
> +			 *
> +			 * Only unmap scpnt's DMA buffers if QDIO is already
> +			 * down.  That is the exact precondition enforced by
> +			 * zfcp_fsf_req_dismiss_all() (BUG_ON QDIOUP) and
> +			 * guarantees the hardware is no longer using them.
> +			 *
> +			 * If QDIO is still up the hardware may still be
> +			 * writing into the buffers; we must not unmap now.
> +			 * Return FAILED so scsi_eh escalates instead of
> +			 * finishing scpnt.  old_req->data stays NULL, so
> +			 * the eventual dismiss will skip the unmap (accepted
> +			 * DMA mapping leak for this path).
> +			 */
>  			zfcp_dbf_scsi_abort("abrt_bl", scpnt, NULL);
> -			return ret;
> +			if (atomic_read(&adapter->status) &
> +			    ZFCP_STATUS_ADAPTER_QDIOUP) {
> +				retval = FAILED;
> +				return retval;
> +			}
> +			retval = ret; /* FAST_IO_FAIL, QDIO is down */
> +			goto out_unmap;
>  		}

[Severity: Critical]
When fc_block_scsi_eh() returns FAST_IO_FAIL while
ZFCP_STATUS_ADAPTER_QDIOUP is set, zfcp_scsi_eh_abort_handler() returns
FAILED here so that scsi_eh escalates instead of finishing scpnt while the
FCP channel may still be performing DMA.

However, when scmd_eh_abort_handler() escalates to
zfcp_scsi_eh_device_reset_handler() or zfcp_scsi_eh_target_reset_handler(),
both call zfcp_scsi_task_mgmt_function():

drivers/s390/scsi/zfcp_scsi.c:zfcp_scsi_task_mgmt_function() {
	while (retry--) {
		fsf_req = zfcp_fsf_fcp_task_mgmt(sdev, tm_flags);
		if (fsf_req)
			break;

		zfcp_dbf_scsi_devreset("wait", sdev, tm_flags, NULL);
		zfcp_erp_wait(adapter);
		ret = fc_block_rport(rport);
		if (ret) {
			zfcp_dbf_scsi_devreset("fiof", sdev, tm_flags, NULL);
			return ret;
		}
}

If zfcp_fsf_fcp_task_mgmt() also returns NULL because the LUN is still
blocked, fc_block_rport() immediately returns FAST_IO_FAIL, and
zfcp_scsi_task_mgmt_function() returns FAST_IO_FAIL directly to
scsi_eh_bus_device_reset() or scsi_eh_target_reset() without checking
ZFCP_STATUS_ADAPTER_QDIOUP and without calling zfcp_scsi_forget_cmnds().

In scsi_eh_bus_device_reset() and scsi_eh_target_reset(), a FAST_IO_FAIL
return is treated the same as SUCCESS, moving all commands for that LUN or
target to done_q where scsi_eh_flush_done_q() calls scsi_finish_command()
and scsi_free_sgtables().

Won't this finish scpnt and free its scatter-gather tables and I/O pages
while QDIO is still up and the hardware may still be performing DMA?
In addition, for any other command on that LUN or target that reached
LUN/target reset without going through zfcp_scsi_eh_abort_handler() (for
example, if scsi_host_eh_past_deadline() skipped abort or
SCSI_EH_ABORT_SCHEDULED was already set on a retry), old_req->data remains
non-NULL, so a subsequent completion or ERP dismissal in
zfcp_fsf_fcp_cmnd_handler() will dereference the freed scpnt and its
scpnt->sdb.table.

>  		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) {
> +		/*
> +		 * abrt_ar: all retries of zfcp_fsf_abort_fcp_cmnd() failed.
> +		 * The hardware may still be running the original I/O; do not
> +		 * unmap.  old_req->data stays NULL so any eventual completion
> +		 * or ERP dismissal of old_req is a harmless no-op.
> +		 */
>  		zfcp_dbf_scsi_abort("abrt_ar", scpnt, NULL);
>  		return FAILED;
>  	}
> @@ -219,11 +278,49 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  	else if (abrt_req->status & ZFCP_STATUS_FSFREQ_ABORTNOTNEEDED)
>  		dbf_tag = "abrt_nn";
>  	else {
> -		dbf_tag = "abrt_fa";
> -		retval = FAILED;
> +		/*
> +		 * abrt_fa: the abort FSF request itself was dismissed by ERP
> +		 * (neither ABORTSUCCEEDED nor ABORTNOTNEEDED is set).  We do
> +		 * not know whether the hardware has stopped the original I/O;
> +		 * do not unmap.  old_req->data stays NULL.
> +		 */
> +		zfcp_dbf_scsi_abort("abrt_fa", scpnt, abrt_req);
> +		zfcp_fsf_req_free(abrt_req);
> +		return FAILED;
>  	}

[ ... ]

> @@ -237,13 +334,17 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
>  {
>  	struct zfcp_scsi_req_filter *filter =
>  		(struct zfcp_scsi_req_filter *)data;
> +	struct scsi_cmnd *scpnt;
>  
> -	/* already aborted - prevent side-effects - or not a SCSI command */
> +	/* already aborted - prevent side-effects - or not a SCSI I/O command */
>  	if (old_req->data == NULL ||
>  	    zfcp_fsf_req_is_status_read_buffer(old_req) ||
> -	    old_req->qtcb->header.fsf_command != FSF_QTCB_FCP_CMND)
> +	    old_req->qtcb->header.fsf_command != FSF_QTCB_FCP_CMND ||
> +	    old_req->handler == zfcp_fsf_fcp_task_mgmt_handler)
>  		return;
>  
> +	scpnt = old_req->data;
> +
>  	/* (tmf_scope == FCP_TMF_TGT_RESET || tmf_scope == FCP_TMF_LUN_RESET) */
>  	if (old_req->qtcb->header.port_handle != filter->port_handle)
>  		return;
> @@ -253,6 +354,17 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
>  		return;
>  
>  	zfcp_dbf_scsi_nullcmnd((struct scsi_cmnd *)old_req->data, old_req);
> +
> +	if ((scsi_get_prot_op(scpnt) != SCSI_PROT_NORMAL) &&
> +	    scsi_prot_sg_count(scpnt))
> +		zfcp_fsf_req_unmap_sg(&old_req->adapter->ccw_device->dev,
> +				      &scpnt->prot_sdb->table,
> +				      scpnt->sc_data_direction);
> +
> +	zfcp_fsf_req_unmap_sg(&old_req->adapter->ccw_device->dev,
> +			      &scpnt->sdb.table,
> +			      scpnt->sc_data_direction);
> +
>  	old_req->data = NULL;
>  }

[Severity: High]
When zfcp_scsi_eh_abort_handler() clears old_req->data = NULL on entry and
then returns FAILED on the abrt_ar, abrt_fa, or abrt_bl (with QDIOUP) paths
without unmapping scpnt->sdb.table and scpnt->prot_sdb->table, how are those
DMA mappings cleaned up when SCSI error recovery escalates?

In scmd_eh_abort_handler(), a FAILED return from eh_abort_handler does not
finish scpnt; instead it calls scsi_eh_scmd_add(scpnt) to escalate recovery
to LUN reset, target reset, and host reset in scsi_unjam_host().

Because old_req->data was already set to NULL:
- If LUN reset or target reset succeeds in zfcp_scsi_task_mgmt_function(),
  zfcp_scsi_forget_cmnds() -> zfcp_scsi_forget_cmnd() sees
  old_req->data == NULL and returns early without unmapping scpnt.
- If host reset (zfcp_scsi_eh_host_reset_handler()) or adapter ERP runs
  zfcp_fsf_req_dismiss_all(), or if old_req completes before reset,
  zfcp_fsf_fcp_cmnd_handler() sees req->data == NULL and returns early
  without unmapping scpnt (and on the abrt_fa path where ERP dismissed
  abrt_req, old_req was already dismissed and freed with data == NULL).

When scsi_eh_flush_done_q() then runs after the reset succeeds, won't it
either requeue scpnt via scsi_queue_insert() and call dma_map_sgtable() a
second time on the still-mapped scpnt->sdb.table in zfcp_fsf_fcp_cmnd(), or
call scsi_finish_command() and leak the DMA mappings and SWIOTLB bounce
buffer slots?

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