[PATCH net v7 0/4] can: j1939: fix the abort timer and bound EDPO

[Chaosheng Qu]

v7 of the j1939 EDPO fix.  Two findings from the automated review of v6 were
real and both were in the session cancel path, so this is now a series:
patch 1 is the timer fix, patch 2 is the original EDPO bound check.

  [1/4] arms the abort timer under the session list lock.  Fixes two things
        at once: the hrtimer_cancel()/j1939_tp_rxtimer() deadlock, and the
        reference leak when a second unparsable frame re-enters the cancel
        path after the timers were killed.

  [2/4] is the original change: reject an ETP.CM_DPO that names a packet at
        or past pkt.total.  Without it a peer can move pkt.dpo past the end
        of the reassembled buffer, and the local EOMA loopback then hands a
        NULL skb to j1939_sk_recv().

  [3/4] guards j1939_session_completed() against that NULL anyway.  With
        [2/4] in place this should be unreachable; it turns a future
        regression in the offset arithmetic into a dropped datagram instead
        of a kernel panic.

  [4/4] rejects an oversized ETP.CM_CTS at the point where it is parsed,
        instead of letting it travel back as a bad EDPO offset.

Testing:

  - [2/4] alone was verified against a local vcan setup: an ETP session with
    ETP.CM_RTS size 1786 (256 packets) followed by ETP.CM_DPO packet 256 and
    an ETP.DT with sequence 0 gave a NULL dereference in j1939_sk_recv()
    3/3 times before the change and was rejected 3/3 times after it.

  - The T5/T6 hang from the review ([1/4]): two unparsable ETP.DT frames
    followed by "ip link del" left unregister_netdevice() waiting for the
    session reference; with the rearmed timer the session is deactivated
    500 ms after the second frame.

  - Each patch builds standalone; net/can/j1939/ is unchanged in behaviour
    for anything that does not involve an EDPO, a CTS or an abort.

Changes in v7:

  - Split into four patches, as promised in the reply to the v6 review.
  - [1/4] replaces the "waiting" branch with rearming the abort timer under
    the lock.  Re-checked the lock order: none of the callers of
    j1939_tp_set_rxtimeout() hold active_session_list_lock, so taking it for
    the rearm does not introduce a new ordering.
  - [2/4] uses J1939_XTP_ABORT_BAD_EDPO_OFFSET for the receiver, which is
    what the comment next to the definition describes and which had no
    callers; J1939_XTP_ABORT_ECTS_TOO_BIG is left to [4/4].
  - Revision history moved below the --- line, Author and Signed-off-by now
    match, and Closes: replaces Link: after Reported-by:.

Bounding the CTS packet number is [4/4] rather than part of [2/4], because
it addresses a different input and can be reviewed on its own.

Chaosheng Qu (4):
  can: j1939: arm the abort timer under the session list lock
  can: j1939: validate the EDPO packet number
  can: j1939: drop the message when the reassembled skb is gone
  can: j1939: reject a CTS that names more packets than the message has

 net/can/j1939/transport.c | 84 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 5 deletions(-)