Re: [PATCH v3 2/4] scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

[Laurence Oberman <loberman@redhat.com>]

On Mon, 2026-10-26 at 13:13 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Critical] scsi: target: iscsi: UAF of OOO cmd in
> iscsit_check_dataout_payload()
> --
> 
> commit 93e4d74df05b6ac07e171f5586d09d2036dc2494
> Author: Laurence Oberman <loberman@redhat.com>
> 
> scsi: target: iscsi: Complete an aborted WRITE when a new R2T would
> be sent
> 
> This commit completes an aborted WRITE command via
> iscsit_complete_aborted_cmd() in iscsit_check_dataout_payload() when
> iscsit_check_post_dataout() returns DATAOUT_SEND_R2T and no
> outstanding R2Ts
> remain.
> 
> > diff --git a/drivers/target/iscsi/iscsi_target.c
> > b/drivers/target/iscsi/iscsi_target.c
> > index aab9237a67768..3dad7f69f3bae 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -1669,6 +1669,22 @@ iscsit_check_dataout_payload(struct
> > iscsit_cmd *cmd, struct iscsi_data *hdr,
> >  	if ((rc == DATAOUT_NORMAL) || (rc ==
> > DATAOUT_WITHIN_COMMAND_RECOVERY))
> >  		return 0;
> >  	else if (rc == DATAOUT_SEND_R2T) {
> > +		/*
> > +		 * A new R2T would never be answered for an
> > aborted command.
> > +		 * The CMD_T_ABORTED handling in
> > __iscsit_check_dataout_hdr()
> > +		 * does not cover this: it only runs for DataOut
> > PDUs
> > +		 * belonging to R2Ts that were already issued, and
> > +		 * iscsit_dataout_update_r2t() has already
> > decremented
> > +		 * outstanding_r2ts for the burst that just
> > finished.  Guard
> > +		 * on outstanding_r2ts so that with
> > MaxOutstandingR2T > 1 the
> > +		 * completion is still left to the last R2T to
> > drain.
> > +		 */
> > +		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> > +			if (cmd->outstanding_r2ts < 1)
> > +				iscsit_complete_aborted_cmd(cmd);
> > +			return 0;
> > +		}
> 
> [Severity: Critical]
> Can completing the command here cause a use-after-free of cmd if
> ICF_OOO_CMDSN is set?
> 
> When ImmediateData=Yes and InitialR2T=No, a WRITE command can arrive
> with
> immediate data and expect additional unsolicited DataOut PDUs in the
> first
> burst followed by R2T-solicited data (payload_length <
> FirstBurstLength <
> se_cmd.data_length, so cmd->unsolicited_data == 1 and
> ICF_NON_IMMEDIATE_UNSOLICITED_DATA is set).
> 
> In drivers/target/iscsi/iscsi_target.c:iscsit_process_scsi_cmd(),
> transport_generic_new_cmd(&cmd->se_cmd) is called before CmdSN
> sequencing:
> 
> 	cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd);
> 	if (cmd->sense_reason)
> 		return 1;
> 
> This places cmd->se_cmd on dev->queues[cpuid].state_list in
> TRANSPORT_WRITE_PENDING. When iscsit_get_immediate_data()
> subsequently calls
> iscsit_sequence_cmd() and cmd->cmd_sn > sess->exp_cmd_sn,
> drivers/target/iscsi/iscsi_target_erl1.c:iscsit_handle_ooo_cmdsn()
> queues
> an entry on sess->sess_ooo_cmdsn_list holding a raw pointer to cmd:
> 
> 	cmd->cmd_flags			|= ICF_OOO_CMDSN;
> 	...
> 	ooo_cmdsn->cmd			= cmd;
> 
> Because cmd->unsolicited_data is set, iscsit_get_immediate_data()
> then calls
> iscsit_set_unsolicited_dataout(cmd).
> 
> If a concurrent TMR aborts cmd while it is still waiting on
> sess->sess_ooo_cmdsn_list, __target_check_io_state() sets
> CMD_T_ABORTED on
> cmd->se_cmd. When the final unsolicited DataOut PDU of the first
> burst
> arrives, __iscsit_check_dataout_hdr() takes the cmd->unsolicited_data
> == 1
> path, and iscsit_dataout_post_crc_passed() clears cmd-
> >unsolicited_data = 0
> and returns DATAOUT_SEND_R2T.
> 
> In this new DATAOUT_SEND_R2T branch in
> iscsit_check_dataout_payload(),
> cmd->outstanding_r2ts is 0, so iscsit_complete_aborted_cmd(cmd) is
> called
> without checking ICF_OOO_CMDSN or removing cmd from
> sess->sess_ooo_cmdsn_list (unlike the adjacent
> DATAOUT_SEND_TO_TRANSPORT
> branch, which checks cmd->cmd_flags & ICF_OOO_CMDSN).
> 
> Once target_complete_cmd() schedules target_abort_work ->
> target_handle_abort(), cmd is completed and freed via
> iscsit_release_cmd().
> When the missing ExpCmdSN PDU later arrives,
> drivers/target/iscsi/iscsi_target_erl1.c:iscsit_execute_ooo_cmdsns()
> will
> dereference the dangling ooo_cmdsn->cmd pointer:
> 
> 		cmd = ooo_cmdsn->cmd;
> 		cmd->i_state = cmd->deferred_i_state;
> 		ooo_count++;
> 		sess->exp_cmd_sn++;
> 		...
> 		iscsit_remove_ooo_cmdsn(sess, ooo_cmdsn);
> 
> 		if (iscsit_execute_cmd(cmd, 1) < 0)
> 			return -1;
> 
> Should the DATAOUT_SEND_R2T abort path also account for commands with
> ICF_OOO_CMDSN set before completing cmd?

I will drop patch 2 for this series as this turns out out be a real
issue and follow up with a v4 for the series. Dropping patch 2 will not
affect the validity of the fix with the over 300 reset loop tests we
ran which resolved the original customer issue.

Sashiko complaints for patches 1 and 4 are pre-existing and wont be
handled in this series.

It has taken many weeks to get to stable outcome for a customer that
included multiple long vmcore analysis tasks and instrumentation.


Thanks
Laurence