Re: [PATCH v3 1/4] scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: target: iscsi: UAF on cmd after iscsit_sequence_cmd() aborts write
--

commit bf534eddda8811708a661c23a1fc1d455c52b52d
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

This commit completes WRITE commands in TRANSPORT_WRITE_PENDING that were
aborted before the first R2T is sent in iscsit_execute_cmd(). It also adds
a completion claim flag under se_cmd.t_state_lock so that only one of
iscsit_execute_cmd() or iscsit_release_commands_from_conn() completes the
command.

> diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
> index d8ca06e697d63..cf72c79be355a 100644
> --- a/drivers/target/iscsi/iscsi_target_erl1.c
> +++ b/drivers/target/iscsi/iscsi_target_erl1.c
> @@ -927,8 +927,16 @@ int iscsit_execute_cmd(struct iscsit_cmd *cmd, int ooo)
>  
>  			if (!(cmd->cmd_flags &
>  					ICF_NON_IMMEDIATE_UNSOLICITED_DATA)) {
> -				if (cmd->se_cmd.transport_state & CMD_T_ABORTED)
> +				/*
> +				 * No R2T has been sent for this command yet,
> +				 * so returning here leaves the initiator with
> +				 * nothing to send and no DataOut timer armed.
> +				 * Complete it, or nothing ever will.
> +				 */
> +				if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> +					iscsit_complete_aborted_cmd(cmd);
>  					return 0;
> +				}

[Severity: High]
Can completing the command here cause a use-after-free on cmd in the
callers of iscsit_sequence_cmd()?

When an iSCSI WRITE arrives with immediate data whose total data length
exceeds FirstBurstLength and with ISCSI_FLAG_CMD_FINAL set on the Command
PDU (so cmd->immediate_data == 1 and cmd->unsolicited_data == 0),
iscsit_process_scsi_cmd() calls transport_generic_new_cmd(), which adds
cmd->se_cmd to dev->queues[cpuid].state_list with cmd_kref == 2 (initial
kref plus SCF_ACK_KREF).

If an ABORT_TASK TMR, a same-session LUN_RESET, or a cross-session
LUN_RESET with emulate_tas == 0 aborts the command before
iscsit_execute_cmd() runs, __target_check_io_state() sets CMD_T_ABORTED
without CMD_T_TAS.

When iscsit_get_immediate_data() or cxgbit_get_immediate_data() then calls
iscsit_sequence_cmd() -> iscsit_execute_cmd(), iscsit_complete_aborted_cmd()
calls target_complete_cmd() -> target_cmd_interrupted(), queueing
target_abort_work on target_completion_wq. On another CPU,
target_abort_work -> target_handle_abort() runs iscsit_aborted_task(),
drops SCF_ACK_KREF via target_put_sess_cmd(), and calls
transport_cmd_check_stop_to_fabric() -> lio_check_stop_free() ->
target_put_sess_cmd() -> iscsit_release_cmd() -> target_free_tag(), which
returns the cmd slot to se_sess->sess_tag_pool.

However, after iscsit_sequence_cmd() returns CMDSN_NORMAL_OPERATION (0),
both drivers/target/iscsi/iscsi_target.c:iscsit_get_immediate_data() and
drivers/target/iscsi/cxgbit/cxgbit_target.c:cxgbit_get_immediate_data()
still dereference cmd without holding a reference on cmd->se_cmd:

drivers/target/iscsi/iscsi_target.c:iscsit_get_immediate_data() {
    ...
		cmdsn_ret = iscsit_sequence_cmd(cmd->conn, cmd,
					(unsigned char *)hdr, hdr->cmdsn);
		if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
			return -1;

		if (cmd->sense_reason || cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
			target_put_sess_cmd(&cmd->se_cmd);

			return 0;
		} else if (cmd->unsolicited_data)
			iscsit_set_unsolicited_dataout(cmd);
    ...
}

If the tag slot has already been reallocated from se_sess->sess_tag_pool,
wouldn't reading cmd->sense_reason or cmd->unsolicited_data inspect the
new command and potentially call target_put_sess_cmd() or
iscsit_set_unsolicited_dataout() on the reallocated command?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026124344.745124-1-loberman@redhat.com?part=1