[PATCH net 0/2] can: j1939: fix NULL deref on ETP completion with out-of-range DPO

[PATCH net 0/2] can: j1939: fix NULL deref on ETP completion with out-of-range DPO

[Xue Boyang]

Hello,

while auditing the J1939 transport layer of v7.3-rc3 we found a NULL
pointer dereference in j1939_session_completed() that is reachable by
an unprivileged user through a virtual CAN interface (all required
capabilities are obtainable inside a user namespace, no hardware
needed).

Root cause: ETP.CM_DPO is accepted without validation, and the final
message distribution looks up the receive queue at pkt.dpo * 7.  With
pkt.dpo moved past the end of the reassembled buffer the lookup
returns NULL, which is passed to j1939_sk_recv() and dereferenced.
The interesting part is that the transfer still *completes normally*
beforehand: TP.DT placement uses "dat[0] - 1 + pkt.dpo" arithmetic,
so with pkt.dpo == pkt.total a final DT frame with dat[0] == 0 is
accepted as the last in-order packet.  The trigger is therefore fully
deterministic - no race, no memory pressure:

    ETP.CM_RTS (size 1786)  -> total = 256 packets
    ETP.CM_DPO (packet 0)
    ETP.DT x255             -> packets 0..254, rx = 255
    ETP.CM_DPO (packet 256) -> dpo = 256, unvalidated
    ETP.DT (dat[0] = 0)     -> packet 255 == rx, completes transfer

    KASAN: null-ptr-deref in range [0x18-0x1f]
    RIP: 0010:j1939_sk_recv+0xd8/0x4b0
    Call Trace:
     j1939_xtp_rx_eoma+0x43d/0x500
     j1939_tp_recv+0x930/0xca0
     j1939_can_recv+0x696/0x900

Both patches verified on v7.3.0-rc3-00313-gdaf677c2c644 + KASAN under
QEMU: the oops reproduces on vanilla, disappears with the series
applied, and the reproducer then completes normally (session
completes, message delivered once with correct dpo).  Self-contained
C reproducer available on request.

Patch 1 fixes the oops by checking the return value of
j1939_session_skb_get() like the only other caller
(j1939_simple_txnext()) already does, while still running
j1939_session_deactivate_activate_next() so the session is not left
stuck.

Patch 2 is a small hardening on top: reject DPO packet numbers that
point past the end of the transfer (pkt.dpo > pkt.total) with an
abort, mirroring the error handling of j1939_xtp_rx_dat_one().  Note
that pkt.dpo == pkt.total combined with dat[0] == 0 still encodes the
final packet and is intentionally kept working - patch 1 is what
actually covers that case.  Happy to drop or rework patch 2 if you
prefer a different boundary.

One design question for the maintainers: the DT placement offset
(dat[0] - 1 + pkt.dpo) and the completion lookup offset (pkt.dpo * 7)
use different semantics for the same field.  It works because window
rebases advance pkt.dpo in lockstep with the packet counter, but it
is the source of this bug class.  A future cleanup could store the
byte offset once, or clamp at DPO-receive time.

Xue Boyang (2):
  can: j1939: fix null-ptr-deref in j1939_session_completed()
  can: j1939: validate the DPO packet number

 net/can/j1939/transport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH net 1/2] can: j1939: fix null-ptr-deref in j1939_session_completed()

[Xue Boyang]

j1939_session_completed() distributes the reassembled message to all
matching receivers via j1939_sk_recv().  The skb is obtained from
j1939_session_skb_get(), which looks up the receive queue starting at
session->pkt.dpo * 7 and returns NULL if no queued skb covers that
offset.

The DPO (Data Packet Offset) command is accepted without validation in
j1939_xtp_rx_dpo_one(), so a peer can move pkt.dpo to a packet number
beyond the end of the reassembled buffer.  The TP.DT placement uses
"dat[0] - 1 + session->pkt.dpo" arithmetic, so with pkt.dpo ==
pkt.total a final DT frame with dat[0] == 0 is still accepted as the
last in-order packet (packet == rx) and the transfer completes
normally.  j1939_session_completed() then looks up the offset
pkt.total * 7, which is outside the receive buffer, so
j1939_session_skb_get() returns NULL and it is passed to
j1939_sk_recv(), which dereferences it via j1939_skb_to_cb().

This is reachable by an unprivileged user through a virtual CAN
interface (all required capabilities are obtainable in a user
namespace) with the frame sequence:

    ETP.CM_RTS (size 1786)  -> total = 256 packets
    ETP.CM_DPO (packet 0)
    ETP.DT x255             -> packets 0..254, rx = 255
    ETP.CM_DPO (packet 256) -> dpo = 256, unvalidated
    ETP.DT (dat[0] = 0)     -> packet 255 == rx, completes transfer

resulting in:

    KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
    RIP: 0010:j1939_sk_recv+0xd8/0x4b0
    Call Trace:
     <IRQ>
     ? j1939_session_skb_get_by_offset+0x14f/0x2a0
     j1939_xtp_rx_eoma+0x43d/0x500
     j1939_tp_recv+0x930/0xca0
     j1939_can_recv+0x696/0x900
     can_rcv_filter+0x223/0x760
     can_receive+0x222/0x310
     can_rcv+0x204/0x370

(j1939_session_completed() is inlined into j1939_xtp_rx_eoma() in
this build; trace taken from v7.3.0-rc3-00313-gdaf677c2c644 + KASAN.)

The only other caller of j1939_session_skb_get(),
j1939_simple_txnext(), already checks for NULL; do the same here and
skip the distribution while still running
j1939_session_deactivate_activate_next() so the session is not left
stuck.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Xue Boyang <m18335910246@163.com>
---
 net/can/j1939/transport.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..ecf7f245f837 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1244,8 +1244,10 @@ 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);
+		if (se_skb) {
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
-- 
2.53.0

[PATCH net 2/2] can: j1939: validate the DPO packet number

[Xue Boyang]

j1939_xtp_rx_dpo_one() copies the 18-bit packet number of the ETP.CM_DPO
command into session->pkt.dpo without any sanity check.  The value is
later used as a byte offset (pkt.dpo * 7) by
j1939_session_skb_get_by_offset(), both for TP.DT placement (combined
with dat[0] - 1) and for the final message distribution in
j1939_session_completed().

A DPO pointing past the end of the transfer has no valid meaning:
every legal window rebase satisfies pkt.dpo <= pkt.total.  Reject
out-of-range values and abort the session with J1939_XTP_ABORT_FAULT
instead of carrying them around, mirroring the error handling of
j1939_xtp_rx_dat_one().

Note that pkt.dpo == pkt.total combined with a DT frame of
dat[0] == 0 still encodes the final packet (dat[0] - 1 + pkt.dpo ==
pkt.total - 1) and therefore stays accepted by design; the NULL
handling in j1939_session_completed() from the previous patch covers
this case.  This patch only discards the clearly bogus values, so the
receive path no longer operates on offsets far outside the buffer.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Xue Boyang <m18335910246@163.com>
---
 net/can/j1939/transport.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index ecf7f245f837..b09983d9c19c 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1833,6 +1833,11 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 
 	/* transmitted without problems */
 	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	if (session->pkt.dpo > session->pkt.total) {
+		j1939_session_timers_cancel(session);
+		j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);
+		return;
+	}
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 
-- 
2.53.0