Re: [PATCH] scsi: target: iscsi: do not reject the command on a Data-Out overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Critical] scsi: target: iscsi: NULL deref in isert_post_recv() via
  iscsit_add_reject()
--

commit b4583fae14731c9492a924f04662a5a7049c19bd
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: target: iscsi: do not reject the command on a Data-Out overflow

This commit changes __iscsit_check_dataout_hdr() to reject an overflowing
Data-Out PDU with iscsit_add_reject() instead of iscsit_reject_cmd() so
that the command's reference is not dropped while it remains on
conn_cmd_list.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 03d618cd76721..884c9678b4ed9 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1485,7 +1485,12 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf,
>  		pr_err("DataOut Offset: %u, Length %u greater than iSCSI Command EDTL %u, protocol error.\n",
>  		       be32_to_cpu(hdr->offset), payload_length,
>  		       cmd->se_cmd.data_length);
> -		return iscsit_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_INVALID, buf);
> +		/*
> +		 * Reject the PDU, not the command: iscsit_reject_cmd() would
> +		 * drop its last reference while it is on conn_cmd_list.
> +		 */
> +		return iscsit_add_reject(conn, ISCSI_REASON_BOOKMARK_INVALID,
> +					 buf);
>  	}
>  
>  	if (cmd->unsolicited_data) {

[Severity: Critical]
Does this cause a NULL pointer dereference in isert_post_recv() on iSER
connections when an unsolicited Data-Out PDU overflows the command's EDTL?

isert_handle_iscsi_dataout() calls iscsit_check_dataout_hdr() ->
__iscsit_check_dataout_hdr().  iscsit_add_reject() allocates a fresh
command with iscsit_allocate_cmd() rather than isert_allocate_cmd(),
leaving the transport-private struct isert_cmd zeroed with rx_desc,
iscsit_cmd, and conn all NULL, and returns -1 so
isert_handle_iscsi_dataout() returns without reposting the incoming
rx_desc.

When the TX thread processes the queued ISTATE_SEND_REJECT response via
isert_response_queue() -> isert_put_reject() -> isert_post_response(), it
passes that NULL isert_cmd->rx_desc to isert_post_recv():

drivers/infiniband/ulp/isert/ib_isert.c:isert_post_response() {
	ret = isert_post_recv(isert_conn, isert_cmd->rx_desc);
	...
}

drivers/infiniband/ulp/isert/ib_isert.c:isert_post_recv() {
	if (!rx_desc->in_use) {
	...
}

This dereferences rx_desc when it is NULL, leaks the original receive
descriptor, and leaves isert_cmd->iscsit_cmd and isert_cmd->conn NULL for
isert_send_done() and isert_do_control_comp().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114161931.941441-1-yhlee@isslab.korea.ac.kr?part=1