SUNRPC: Validate TCP record marker length before trusting it

From: Chu Zhou <[email protected]>
Subject: [PATCH] SUNRPC: Validate TCP record marker length before trusting it

The SUNRPC/TCP receive parser trusts the 31-bit fragment length of the record marker (RFC 5531) unconditionally. If the byte stream ever loses framing sync - for example due to corruption below the RPC layer (driver / checksum offload / DMA coherence bugs seen on embedded platforms) or a malfunctioning peer - arbitrary payload bytes are interpreted as a record marker, and the parser can latch onto a huge bogus fragment length.

Observed in the field on an OrangePi 5 Plus (Rockchip RK3588) NFSv3/TCP
client: the parser latched onto reclen=0x4DB5ED05 (~1.24 GiB) and entered the MSG_TRUNC discard path. From that point on, every genuine reply was discarded as payload of the bogus fragment, with recv.offset advancing one skb at a time toward a 1.24 GiB boundary that does not exist in the stream. The TCP connection stayed ESTABLISHED and data kept flowing, so no socket error or state change ever triggered a transport reset, and record marking has no resynchronization mechanism. All RPC tasks piled up in uninterruptible sleep until the hung task watchdog panicked the machine. Restarting the NFS server (which tears down the connection) was the only way to recover.

The set of legitimate fragment lengths has a hard upper bound: NFS over TCP negotiates message sizes of at most 1 MiB (rsize/wsize). Treat any length above 16 MiB (a generous 16x margin) as proof of framing desync, emit a tracepoint that captures the parser state for post-mortem analysis, and fail the read with -ESHUTDOWN so that the connection is torn down and re-established by the existing transport reset machinery.

The check costs one comparison per fragment.

---
 include/trace/events/sunrpc.h | 30 ++++++++++++++++++++++++++++++
 net/sunrpc/xprtsock.c         | 14 ++++++++++++++
 2 files changed, 44 insertions(+)

index ff85519..d7e2b10 100644
@@ -1370,6 +1370,36 @@ TRACE_EVENT(xs_stream_read_request,
 			__entry->copied, __entry->reclen, __entry->offset)
 );
 
+TRACE_EVENT(xs_stream_bogus_marker,
+	TP_PROTO(const struct sock_xprt *xs),
+
+	TP_ARGS(xs),
+
+	TP_STRUCT__entry(
+		__string(addr, xs->xprt.address_strings[RPC_DISPLAY_ADDR])
+		__string(port, xs->xprt.address_strings[RPC_DISPLAY_PORT])
+		__field(u32, fraghdr)
+		__field(u32, xid)
+		__field(unsigned int, reclen)
+		__field(unsigned int, offset)
+		__field(long, copied)
+	),
+
+	TP_fast_assign(
+		__assign_str(addr);
+		__assign_str(port);
+		__entry->fraghdr = be32_to_cpu(xs->recv.fraghdr);
+		__entry->xid = be32_to_cpu(xs->recv.xid);
+		__entry->reclen = xs->recv.len;
+		__entry->offset = xs->recv.offset;
+		__entry->copied = (long)xs->recv.copied;
+	),
+
+	TP_printk("peer=[%s]:%s fraghdr=0x%08x reclen=%u xid=0x%08x offset=%u copied=%ld",
+		__get_str(addr), __get_str(port), __entry->fraghdr,
+		__entry->reclen, __entry->xid, __entry->offset, __entry->copied)
+);
+
 TRACE_EVENT(rpcb_getport,
 	TP_PROTO(
 		const struct rpc_clnt *clnt,

index 7f60723..cd03dc6 100644
@@ -80,6 +80,15 @@ static unsigned int xprt_max_resvport = RPC_DEF_MAX_RESVPORT;
 #define XS_TCP_LINGER_TO	(15U * HZ)
 static unsigned int xs_tcp_fin_timeout __read_mostly = XS_TCP_LINGER_TO;
 
+/*
+ * Largest RPC record marker length we are prepared to trust.
+ * NFS over TCP negotiates message sizes of at most 1 MiB (rsize/wsize),
+ * so this leaves a generous margin. A larger value means the stream
+ * parser has lost framing sync; the only recovery is to reset the
+ * connection, since record marking has no resynchronization mechanism.
+ */
+#define XS_MAX_RECORD_LEN	(16U << 20)
+
 /*
  * We can register our own files under /proc/sys/sunrpc by
  * calling register_sysctl() again.  The files in that

@@ -713,6 +722,11 @@ xs_read_stream(struct sock_xprt *transport, int flags)
 			return transport->recv.offset;
 		transport->recv.len = be32_to_cpu(transport->recv.fraghdr) &
 			RPC_FRAGMENT_SIZE_MASK;
+		if (unlikely(transport->recv.len > XS_MAX_RECORD_LEN)) {
+			trace_xs_stream_bogus_marker(transport);
+			ret = -ESHUTDOWN;
+			goto out_err;
+		}
 		transport->recv.offset -= sizeof(transport->recv.fraghdr);
 		read = ret;
 	}

Ping - three weeks on with no review or ack.

Re-verified this morning: it still applies cleanly to current mainline (v7.3-rc7), diffstat unchanged at 2 files, 44 insertions.

https://lore.kernel.org/linux-nfs/[email protected]/

I went back over the error path end to end, since the whole mechanism rests on it. xs_read_stream()'s out_err label is

	return ret != 0 ? ret : -ESHUTDOWN;

and at the point the new check fires, ret still holds the positive byte count returned by xs_read_stream_header(), so the patch assigns -ESHUTDOWN explicitly before branching there. That value then reaches xs_stream_data_receive(), which calls kernel_sock_shutdown(transport->sock, SHUT_RDWR), and the existing transport reset machinery re-establishes the connection as described in the commit message.

Worth restating that the failure mode is a hard hang, not a throughput problem. The parser latched onto a bogus reclen after below-RPC corruption, then discarded every genuine reply through the MSG_TRUNC path while the TCP connection stayed ESTABLISHED - nothing ever triggered a reset, and the hung task watchdog panicked the machine. Restarting the server was the only recovery.

Two-line check plus a tracepoint, one comparison per fragment. The 16 MiB bound is a judgement call - happy to lower it or tie it to a named constant if that is preferred.

Thanks,