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

[Quchaosheng]

v6 addresses both High findings, the Medium, and the two Low comments.

> [High] can j1939_session_cancel() deadlock against the rxtimer callback?

Yes, and j1939_tp_txtimer()'s -EOVERFLOW and default branches reach it
without cancelling the rxtimer first.  The fix is in the helper:

  j1939_session_cancel()
    j1939_session_list_lock()
    j1939_tp_set_rxtimeout()
      j1939_session_rxtimer_cancel()
        hrtimer_cancel()      <- waits for a callback that wants our lock

v6 cancels the rxtimer before taking the lock, then arms the abort timer
with hrtimer_start() under the lock.  hrtimer_start() does not wait for the
callback, so the cancel/arm ordering the callers rely on is preserved
without the synchronous cancel inside the critical section.

> [High] What happens if the next bad frame is an ETP.DT or an ETP.CM_CTS?

The session is stranded.  The timers are cancelled, j1939_session_cancel()
sees WAITING_ABORT and does nothing, and nothing is left to deactivate it.

v6 handles that in the helper: a session already in WAITING_ABORT is
deactivated there instead of being left for a timer the caller has just
cancelled.  That covers ETP.DT, ETP.CM_CTS and second-RTS without touching
any of them.  In j1939_xtp_rx_rts_session_active() the receiver branch then
does what the transmitter branch already did, so both become one
unconditional deactivation.

Measured on v7.3-rc7 under QEMU/KVM on vcan, one tree and one config
(defconfig + CONFIG_CAN_J1939=y, CONFIG_CAN_VCAN=y, CONFIG_LOCKDEP=y,
CONFIG_DEBUG_SPINLOCK=y), three kernels differing only in this hunk.  Each
case is a standalone guest.

             A           B             C           D
  stock      SLOT-STUCK  REF-DROPPED*  SLOT-STUCK  never returns
  v5         SLOT-STUCK  never returns SLOT-STUCK  never returns
  v6         SLOT-FREE   returns       SLOT-FREE   returns

  A  out-of-range EDPO, then an unparsable DT; is a second ETP.CM_RTS from
     the same SA/DA answered with a CTS?
  B  same as A, last socket closed, does `ip link del vcan0` return?
  C  two unparsable DTs, no EDPO; second RTS answered?
  D  two unparsable DTs; does `ip link del vcan0` return?

* B on stock aborts with code 6, UNEXPECTED_DATA, which leaves the session
  to the abort timer, and the timer deactivates it as the device goes down.
  A is still stuck on stock.

C and D are the case you described.  On stock the second unparsable DT
cancels the abort timer the first one armed, so the session stays on
active_session_list; D prints "unregister_netdevice: waiting for vcan0 to
become free. Usage count = 7" until the guest is killed.  Pre-existing, as
you said.  v5 fixed only the caller it added, so A through D are unchanged
from stock.

> [Medium] The transmitter role sends BAD_EDPO_OFFSET, and the commit
> message does not match the code

Both correct.  v6 sends J1939_XTP_ABORT_ECTS_TOO_BIG for a transmission
session and J1939_XTP_ABORT_BAD_EDPO_OFFSET for a receiver.  With a real
J1939 sender and an injected oversized CTS, v5 puts 12 on the wire and v6
puts 15.

The commit message now says what the code does:

  - tx_acked is pkt - 1 and is not truncated when written to dat[2..4];
    only dat[1] wraps.  "An offset that cannot be encoded" was wrong.
  - the check runs on the echo, after j1939_session_tx_dpo() has put the
    EDPO on the bus.  Before the patch the transmitter did not run with
    that offset either: j1939_session_tx_dat() failed with -ENOBUFS and the
    session ended in -ENETUNREACH.
  - the force-deactivation paragraph now describes the helper.

> [Low] The j1939_session_completed() comment

The example cannot happen with the range check in place, so the comment now
describes the guard as defense in depth.

> [Low] The comment says pkt.dpo "has to stay inside the message", but
> j1939_session_tx_dpo() has another, unbounded writer

The comment now says it bounds the receiver-side writer, names
j1939_session_tx_dpo() and the unchecked ETP.CM_CTS packet number, and
says that is a separate change.

The single-EDPO case and a legal 1786-byte transfer were run on all three
kernels as well: stock panics with "BUG: kernel NULL pointer dereference,
address: 0x18" in j1939_sk_recv(), and v6 delivers all 256 packets intact.

Not reproduced: the lockdep race the rxtimer change fixes.  It needs a timer
callback racing a caller and none of these sequences schedules one at the
right moment.  No lockdep report or call trace appeared in any run.

pw-bot: cr

  net/can/j1939/transport.c | 81 ++++++++++++++++++++++++++++++++++++++++++---------
  1 file changed, 66 insertions(+), 15 deletions(-)