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

[Chaosheng Qu]

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, which the comment next to the definition
describes as "Bad EDPO offset" and which had no callers.  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.

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.

Signed-off-by: Chaosheng Qu <quchaosheng000406@xxxxxxx>
---
 net/can/j1939/transport.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 590a04c453b5..b0e62822b56e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1849,14 +1849,43 @@ 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);
+
+	/* ETP.CM_DPO 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
+	 * beyond the reassembled buffer.  The transfer still completes: with
+	 * pkt.dpo == pkt.total a closing ETP.DT with dat[0] == 0 evaluates to
+	 * the last in-order packet, so pkt.rx reaches pkt.total and the receiver
+	 * emits its own EOMA.  The loopback of that EOMA runs
+	 * j1939_session_completed(), which asks j1939_session_skb_get() for a
+	 * packet past the end of the queued skb, gets NULL, and passes it to
+	 * j1939_sk_recv().
+	 *
+	 * pkt.total is a packet count, so the last valid packet is
+	 * pkt.total - 1 and the check has to be ">=".
+	 */
+	if (dpo >= session->pkt.total) {
+		if (session->transmission) {
+			/* 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.
+			 */
+			j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
+		} else {
+			j1939_session_cancel(session, 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