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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: missing txtimer cancel before
  j1939_session_cancel()
- [Medium] can: j1939: session ref leak in j1939_session_cancel() on
  rxtimer race
- [Low] can: j1939: missing Fixes trailer for EDPO bounds check
--

commit 1f6785035506d85ce93ce6a036332f472884137d
Author: Chaosheng Qu <quchaosheng000406@163.com>

> can: j1939: validate the EDPO packet number
>
> 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@163.com>

[Severity: Low]
Since this fixes a NULL pointer dereference present since J1939 support was
introduced, should it include a Fixes tag:

  Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 590a04c453b56..b0e62822b56e4 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;
> +	}

[Severity: High]
Does aborting here without calling j1939_session_timers_cancel(session) leave
session->txtimer armed?

j1939_session_cancel() only cancels session->rxtimer, which is why the other
receive-path abort handlers (j1939_xtp_rx_cts_one(),
j1939_xtp_rx_rts_session_active() and j1939_xtp_rx_dat_one()) call
j1939_session_timers_cancel(session) first.

On the transmitter path, after j1939_tp_txtimer() sends ETP.CM_DPO with an
out-of-range pkt.tx, j1939_session_tx_dat() sees NULL from
j1939_session_skb_get_by_offset() and returns -ENOBUFS, so j1939_tp_txtimer()
re-arms session->txtimer for 10..25 ms.  When the looped-back ETP.CM_DPO then
hits this check, session->txtimer stays active on the aborting session, retries
up to J1939_XTP_TX_RETRY_LIMIT and overwrites session->err with -ENETUNREACH
(or on a receiver session, transmits CTS or EOMA and clobbers the abort
timeout via j1939_tp_set_rxtimeout()).

[Severity: Medium]
Can calling j1939_session_cancel() here race with a second cancel or timeout
update to double-arm session->rxtimer and leak a struct j1939_session
reference?

In j1939_session_cancel() (modified in commit "can: j1939: arm the abort timer
under the session list lock"), j1939_session_rxtimer_cancel() runs before
taking active_session_list_lock, and then hrtimer_start(&session->rxtimer, ...)
runs unconditionally under the lock whenever abort_sent is true or
session->state == J1939_SESSION_WAITING_ABORT:

CPU0                              CPU1
j1939_xtp_rx_dpo_one()            j1939_xtp_rx_dpo_one()
  j1939_session_cancel()            j1939_session_cancel()
    j1939_session_rxtimer_cancel()    j1939_session_rxtimer_cancel()
    j1939_session_list_lock()
    __j1939_session_cancel()
      state = WAITING_ABORT
    j1939_session_get()
    hrtimer_start(&rxtimer, ...)
    j1939_session_list_unlock()
                                      j1939_session_list_lock()
                                      /* state == WAITING_ABORT */
                                      j1939_session_get()
                                      hrtimer_start(&rxtimer, ...)
                                        <-- overwrites armed timer, leaks ref

j1939_tp_set_rxtimeout() also rearms session->rxtimer without holding
active_session_list_lock, so it can likewise arm session->rxtimer between
j1939_session_rxtimer_cancel() and hrtimer_start().  The leaked kref prevents
j1939_session_destroy() from running, leaking the session and its tracked
net_device reference and hanging unregister_netdevice().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179410166344.60581.3898011861151549156@163.com?part=2