[PATCH] scsi: target: tcm_fc: reject overlapping write-data frames

[PATCH] scsi: target: tcm_fc: reject overlapping write-data frames

[Yehyeong Lee]

ft_recv_write_data() executes a WRITE once the received byte count reaches
se_cmd->data_length.  Frames are placed by their FCP relative offset but
nothing tracks which offsets were filled, so an initiator that sends two
frames at the same offset reaches data_length with part of the buffer never
written: two 4096-byte frames at offset 0 of an 8192-byte write get the
count to 8192 and leave the second half untouched.

The buffer is not zeroed (transport_generic_new_cmd() clears zero_flag for
a data CDB), so that gap keeps the previous page contents, which the
backend then writes to the device for the initiator to read back.  There is
no oops; poisoning the buffer and sending such a pair leaves 4096 bytes of
it committed unchanged, and the patched driver refuses the overlapping
frame.

Track the range received and drop a frame that falls inside it.  With no
overlapping frame accepted, the byte count reaches data_length only when
the whole buffer has been written, so it stays a valid completion test;
in-order and out-of-order complete writes are unaffected.

Fixes: 3699d92a4d7b ("[SCSI] tcm_fc: Adding FC_FC4 provider (tcm_fc) for FCoE target (TCM - target core) support")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/target/tcm_fc/tcm_fc.h |  8 +++++
 drivers/target/tcm_fc/tfc_io.c | 57 ++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
index 00e5573c62966..b75b1373a49c9 100644
--- a/drivers/target/tcm_fc/tcm_fc.h
+++ b/drivers/target/tcm_fc/tcm_fc.h
@@ -103,12 +103,20 @@ struct ft_lport_wwn {
 /*
  * Commands
  */
+#define FT_WRITE_SEGS	4	/* out-of-order write ranges tracked per cmd */
+
 struct ft_cmd {
 	struct ft_sess *sess;		/* session held for cmd */
 	struct fc_seq *seq;		/* sequence in exchange mgr */
 	struct se_cmd se_cmd;		/* Local TCM I/O descriptor */
 	struct fc_frame *req_frame;
 	u32 write_data_len;		/* data received on writes */
+	u32 write_contig_end;		/* end of the contiguous data received */
+	u8 write_nseg;			/* out-of-order ranges held below */
+	struct ft_write_seg {
+		u32 off;
+		u32 end;
+	} write_seg[FT_WRITE_SEGS];
 	struct work_struct work;
 	/* Local sense buffer */
 	unsigned char ft_sense_buffer[TRANSPORT_SENSE_BUFFER];
diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
index 45329284f52f5..53d7c7268865d 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -189,6 +189,56 @@ static void ft_execute_work(struct work_struct *work)
 	target_execute_cmd(&cmd->se_cmd);
 }
 
+/* Record a frame's range; refuse one that repeats data already received. */
+static bool ft_add_write_range(struct ft_cmd *cmd, u32 off, u32 len)
+{
+	u32 end = off + len;
+	unsigned int i, j;
+	bool absorbed;
+
+	if (off < cmd->write_contig_end)
+		return false;
+	for (i = 0; i < cmd->write_nseg; i++)
+		if (off < cmd->write_seg[i].end && cmd->write_seg[i].off < end)
+			return false;
+
+	if (off != cmd->write_contig_end) {
+		for (i = 0; i < cmd->write_nseg; i++) {
+			if (cmd->write_seg[i].end == off) {
+				cmd->write_seg[i].end = end;
+				return true;
+			}
+			if (cmd->write_seg[i].off == end) {
+				cmd->write_seg[i].off = off;
+				return true;
+			}
+		}
+		if (cmd->write_nseg >= FT_WRITE_SEGS)
+			return false;
+		cmd->write_seg[cmd->write_nseg].off = off;
+		cmd->write_seg[cmd->write_nseg].end = end;
+		cmd->write_nseg++;
+		return true;
+	}
+
+	cmd->write_contig_end = end;
+	do {
+		absorbed = false;
+		for (i = 0; i < cmd->write_nseg; i++) {
+			if (cmd->write_seg[i].off != cmd->write_contig_end)
+				continue;
+			cmd->write_contig_end = cmd->write_seg[i].end;
+			for (j = i; j + 1 < cmd->write_nseg; j++)
+				cmd->write_seg[j] = cmd->write_seg[j + 1];
+			cmd->write_nseg--;
+			absorbed = true;
+			break;
+		}
+	} while (absorbed);
+
+	return true;
+}
+
 /*
  * Receive write data frame.
  */
@@ -267,6 +317,11 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct fc_frame *fp)
 		goto drop;
 	if (frame_len + rel_off > se_cmd->data_length)
 		frame_len = se_cmd->data_length - rel_off;
+	if (!ft_add_write_range(cmd, rel_off, frame_len)) {
+		pr_err_ratelimited("%s: xid 0x%x: offset %u len %zu repeats data\n",
+				   __func__, ep->xid, rel_off, frame_len);
+		goto drop;
+	}
 
 	/*
 	 * Setup to use first mem list entry, unless no data.
@@ -345,6 +400,8 @@ void ft_invl_hw_context(struct ft_cmd *cmd)
 				 */
 				cmd->write_data_len = lport->tt.ddp_done(lport,
 								      ep->xid);
+				cmd->write_contig_end = cmd->write_data_len;
+				cmd->write_nseg = 0;
 
 				/*
 				 * Resetting same variable to indicate HW's
-- 
2.43.0

Re: [PATCH] scsi: target: tcm_fc: reject overlapping write-data frames

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tcm_fc: missing segment merge causes premature exhaustion of write_seg array
--

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

scsi: target: tcm_fc: reject overlapping write-data frames

This patch tracks received write ranges for out-of-order FCP frames to
drop frames that repeat already received data. It introduces an array to
hold disconnected segments, preventing overlapping frames from leaving
unwritten gaps in the data buffer.

Fixes: 3699d92a4d7b ("[SCSI] tcm_fc: Adding FC_FC4 provider (tcm_fc) for FCoE target (TCM - target core) support")
Cc: stable@vger.kernel.org

> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> index 45329284f52f5..53d7c7268865d 100644
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c
[ ... ]
> +static bool ft_add_write_range(struct ft_cmd *cmd, u32 off, u32 len)
> +{
[ ... ]
> +	if (off != cmd->write_contig_end) {
> +		for (i = 0; i < cmd->write_nseg; i++) {
> +			if (cmd->write_seg[i].end == off) {
> +				cmd->write_seg[i].end = end;
> +				return true;
> +			}
> +			if (cmd->write_seg[i].off == end) {
> +				cmd->write_seg[i].off = off;
> +				return true;
> +			}

[Severity: Medium]
When an out-of-order write frame extends the boundary of an existing tracked
segment, does returning true immediately bypass any logic to check if this
extension makes the segment contiguous with another tracked segment?

If adjacent segments are not coalesced (unless they directly touch
cmd->write_contig_end), could this tracking array artificially waste slots on
segments that are now adjacent to each other?

> +		}
> +		if (cmd->write_nseg >= FT_WRITE_SEGS)
> +			return false;

[Severity: Medium]
Due to the missing merge logic mentioned above, could the array remain
artificially full of adjacent segments, erroneously triggering this limit
and dropping valid incoming data frames?

If an initiator sends heavily fragmented out-of-order frames, the array
could quickly fill with unmerged adjacent segments. Would subsequent
valid non-adjacent frames then be dropped with a misleading "repeats data"
error, potentially causing FC sequence timeouts, FCP exchange aborts, and
transport layer failures on the initiator?

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

Re: [PATCH] scsi: target: tcm_fc: reject overlapping write-data frames

[Yehyeong Lee]

Thanks.  You are right - v1 extended a tracked range but didn't re-check
whether it had become adjacent to another, so a reordered write could fill
the small array with ranges that were really contiguous and then reject a
later frame as a repeat.  v2 coalesces adjacent ranges after each frame, so
that no longer happens; I reproduced the spurious drop with a reordered
16-frame write and confirmed v2 accepts it.  A write split into more
simultaneous gaps than the array holds is still dropped, but that is a
misbehaving initiator and the exchange just aborts.

v2: https://lore.kernel.org/linux-scsi/20261012153502.2435483-1-yhlee@isslab.korea.ac.kr/

Thanks,
Yehyeong