[PATCH net v8 2/4] can: j1939: validate the EDPO packet number

[Chaosheng Qu <quchaosheng000406@163.com>]

ETP.CM_DPO is peer controlled and its packet number is accepted without
any validation.  It names the packet a transmission window starts at, and
j1939_xtp_rx_dat_one() places incoming data at "dat[0] - 1 + pkt.dpo", so
a value past the end of the message moves pkt.dpo beyond the reassembled
buffer.

The transfer still completes normally in that case.  With
pkt.dpo == pkt.total, a closing ETP.DT frame with dat[0] == 0 evaluates to
packet pkt.total - 1, which is the last in-order packet, so pkt.rx reaches
pkt.total and the receiver emits its own EOMA.  The local loopback of that
EOMA runs j1939_session_completed(), which asks j1939_session_skb_get()
for offset "pkt.dpo * 7" -- one full packet past the end of the queued
skb -- gets NULL, and passes it straight to j1939_sk_recv(), which
dereferences oskb->sk.

Reject an EDPO that names a packet at or past pkt.total and abort the
session.  The check has to be ">=" and not ">": pkt.total is a packet
count, so the last valid packet is pkt.total - 1.  The abort code depends
on the role: a receiver rejects a bad offset with
J1939_XTP_ABORT_BAD_EDPO_OFFSET, while a transmitter only builds an EDPO
from pkt.tx_acked, which comes from the responder's ETP.CM_CTS, so an
out-of-range value there is an oversized CTS and is reported as
J1939_XTP_ABORT_ECTS_TOO_BIG.

Cancel the session timers before aborting, as the other receive-path abort
handlers do.  j1939_session_cancel() only rearms the abort timer and
leaves session->txtimer armed, so on the transmitter path the retry timer
keeps firing on an aborting session, retries up to J1939_XTP_TX_RETRY_LIMIT
and overwrites session->err with -ENETUNREACH; on the receiver path the
retries transmit CTS or EOMA and clobber the abort timeout.

Bounding the CTS packet number in j1939_xtp_rx_cts_one() is a separate
change; this one only stops the value from being used as an offset.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index fa23260e1833..ab42a383ff87 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1853,14 +1853,42 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 				 struct sk_buff *skb)
 {
 	const u8 *dat = skb->data;
+	unsigned int dpo;
 
 	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
 		return;
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	dpo = j1939_etp_ctl_to_packet(dat);
+
+	/* EDPO is peer controlled.  j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so a value at or past pkt.total moves
+	 * pkt.dpo past the reassembled buffer, and the ETP completion path
+	 * then looks up an offset no queued skb covers.
+	 *
+	 * This bounds the receiver-side writer only.  j1939_session_tx_dpo()
+	 * copies pkt.tx_acked, which comes from an ETP.CM_CTS packet number
+	 * that j1939_xtp_rx_cts_one() does not bound; that is a separate fix.
+	 */
+	if (dpo >= session->pkt.total) {
+		netdev_warn(session->priv->ndev,
+			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
+			    __func__, session, dpo, session->pkt.total);
+		j1939_session_timers_cancel(session);
+		/* The transmitter only sees this on the echo of an EDPO it built
+		 * from an oversized ETP.CM_CTS packet number, so it is the
+		 * responder's CTS that is too big, not the EDPO offset.
+		 */
+		j1939_session_cancel(session,
+				     session->transmission ?
+				     J1939_XTP_ABORT_ECTS_TOO_BIG :
+				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);
+		return;
+	}
+
 	/* transmitted without problems */
-	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	session->pkt.dpo = dpo;
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 
-- 
2.43.0