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

[PATCH v2] 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>
---
v2: coalesce adjacent tracked ranges, so a reordered write whose segments
    have become contiguous no longer exhausts the array and is wrongly
    refused (Sashiko).

 drivers/target/tcm_fc/tcm_fc.h |  8 ++++
 drivers/target/tcm_fc/tfc_io.c | 74 ++++++++++++++++++++++++++++++++++
 2 files changed, 82 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..27eb9da0bc01f 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -189,6 +189,73 @@ static void ft_execute_work(struct work_struct *work)
 	target_execute_cmd(&cmd->se_cmd);
 }
 
+/* Fold a range that now adjoins the contiguous front, or another range. */
+static void ft_merge_write_ranges(struct ft_cmd *cmd)
+{
+	unsigned int i, j;
+	bool merged;
+
+	do {
+		merged = false;
+		for (i = 0; i < cmd->write_nseg; i++) {
+			if (cmd->write_seg[i].off == cmd->write_contig_end) {
+				cmd->write_contig_end = cmd->write_seg[i].end;
+			} else {
+				for (j = 0; j < cmd->write_nseg; j++)
+					if (j != i && cmd->write_seg[j].end ==
+					    cmd->write_seg[i].off)
+						break;
+				if (j == cmd->write_nseg)
+					continue;
+				cmd->write_seg[j].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--;
+			merged = true;
+			break;
+		}
+	} while (merged);
+}
+
+/* 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;
+
+	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) {
+		cmd->write_contig_end = end;
+	} else {
+		for (i = 0; i < cmd->write_nseg; i++) {
+			if (cmd->write_seg[i].end == off) {
+				cmd->write_seg[i].end = end;
+				break;
+			}
+			if (cmd->write_seg[i].off == end) {
+				cmd->write_seg[i].off = off;
+				break;
+			}
+		}
+		if (i == cmd->write_nseg) {
+			if (cmd->write_nseg >= FT_WRITE_SEGS)
+				return false;
+			cmd->write_seg[i].off = off;
+			cmd->write_seg[i].end = end;
+			cmd->write_nseg++;
+		}
+	}
+
+	ft_merge_write_ranges(cmd);
+	return true;
+}
+
 /*
  * Receive write data frame.
  */
@@ -267,6 +334,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 +417,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 v2] 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:
- [High] tcm_fc: out-of-order writes fail and timeout due to hardcoded FT_WRITE_SEGS limit
--

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

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

This commit introduces tracking of FCoE write data segments to prevent
overlapping frames from falsely satisfying the command completion length test,
which could otherwise leave trailing buffer contents unchanged. It tracks
disjoint write ranges in a fixed-size array to reject duplicated payload data.

> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c

[ ... ]

> +/* 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;
> +
> +	if (off < cmd->write_contig_end)
> +		return false;

[ ... ]

> +		if (i == cmd->write_nseg) {
> +			if (cmd->write_nseg >= FT_WRITE_SEGS)
> +				return false;
> +			cmd->write_seg[i].off = off;
> +			cmd->write_seg[i].end = end;
> +			cmd->write_nseg++;
> +		}
> +	}
> +
> +	ft_merge_write_ranges(cmd);
> +	return true;
> +}

[Severity: High]
Does this hardcoded limit reject legitimate, highly out-of-order FCoE frames?

If network packet loss or reordering creates 5 or more disjoint data segments
before they can be coalesced by ft_merge_write_ranges(), this code hits the
FT_WRITE_SEGS cap and returns false for a valid frame.

Additionally, is it possible for concurrent receive threads to call
ft_add_write_range() for the same exchange simultaneously?

Since the array manipulation and cmd->write_nseg increments are performed
without locks, concurrent frames could cause a data race, potentially
leading to array out-of-bounds corruption into cmd->work.

[ ... ]

>  	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.

[Severity: High]
If ft_add_write_range() returns false due to hitting the FT_WRITE_SEGS limit,
does this silently drop the frame without aborting the exchange?

Because FCP write data is unacknowledged per-frame, the initiator will not
be aware that the target dropped the data and will wait for command
completion. The target will hang indefinitely waiting for the dropped bytes.

Will this cause the I/O command to permanently hang until it times out under
non-ideal network conditions?

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

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

[Yehyeong Lee]

Thanks for the careful read.

On the lock: a command's write-data frames are not delivered concurrently.
A target write-data frame is a sequence request (EX_CTX/SEQ_CTX clear), so
fcoe_rcv() steers it by rx_id onto one per-CPU queue and fcoe_receive_work()
drains that queue serially; one exchange is one rx_id, hence one CPU and one
work item.  ft_recv_write_data() for a command is therefore single-threaded,
which is why the existing cmd->write_data_len accumulation and the
== se_cmd->data_length test have run unlocked since the driver was added in
3699d92a4d7b.  write_seg[] is updated in the same place under the same
guarantee and adds no new shared state.

On the FT_WRITE_SEGS limit: a lost frame makes the byte count never reach
data_length, so that command already fails without this patch.  The only
case newly affected is pure reordering that keeps more than FT_WRITE_SEGS
disjoint ranges open at once and then completes; the libfc initiator keeps a
single contiguous watermark (fsp->xfer_contig_end), so four tracked ranges is
already more than the in-tree initiator.

On the dropped frame: it takes the same path as the four existing drops in
this function (no REL_OFF, DDP without SEQ_INIT, runt frame, offset past the
buffer) - the frame is freed and the initiator recovers the command by
ABTS/timeout, as for a lost frame; tcm_fc arms no receiver-side timer.  I
would keep that consistent rather than make this one drop abort on its own,
but can add an explicit abort if you prefer.

Thanks,
Yehyeong