Re: [PATCH net v6 1/1] can: j1939: validate the EDPO packet number

[Chaosheng Qu]

Hi sashiko,

Thanks -- both Medium findings are real. I will respin as a split series
rather than argue with them, and the three Low findings are accepted too; two
of them are process points that I should simply have gotten right the first
time.

## Medium: session reference leak in `j1939_session_cancel()`

Accepted. The old code did the cancel/get/start back to back inside
`j1939_tp_set_rxtimeout()`. The patch moves the cancel above
`j1939_session_list_lock()` and the get+start below `__j1939_session_cancel()`,
which calls `j1939_xtp_tx_abort()` -> `j1939_xtp_do_tx_ctl()` ->
`can_send()`, and then `j1939_sk_send_loop_abort()`. So the window now spans
lock acquisition and a CAN frame transmit instead of three instructions.

The list lock does not help, exactly as you say: `j1939_tp_set_rxtimeout()`
does not take it, and it is reached from the receive path without it by
`j1939_xtp_rx_cts_one()`, `j1939_xtp_rx_dpo_one()` and
`j1939_xtp_rx_dat_one()`, plus from the transmit retry path through
`j1939_session_tx_rts()`/`_tx_dpo()`/`_tx_dat()` inside `j1939_tp_txtimer()`.
Your CPU0/CPU1 interleaving is therefore reachable: two `j1939_session_get()`
calls, one queued hrtimer, one `j1939_session_put()` on expiry. The session
then never reaches `j1939_session_destroy()`, so it keeps both its
`priv_dev_tracker` netdev reference and its `j1939_priv` reference, and
`ip link del` hangs in `unregister_netdevice()`.

Two notes on the fix I intend to take, because I do not think
`hrtimer_try_to_cancel()` alone closes it:

- The reason the cancel had to move out of the lock stays valid. Before this
  patch, `j1939_session_cancel()` called `hrtimer_cancel(&session->rxtimer)`
  while holding `active_session_list_lock`, and `j1939_tp_rxtimer()` takes the
  same lock, so a running softirq callback waiting for the lock deadlocks the
  CPU that holds the lock and waits for the callback.
- `hrtimer_try_to_cancel()` under the lock avoids the deadlock -- a callback
  that is already running makes it return -1 instead of waiting -- but the get
  and the start would still race with a concurrent `j1939_tp_set_rxtimeout()`,
  which does its own cancel/get/start without the lock. Closing the window
  needs both armers serialized, so the respin arms the rxtimer only under
  `active_session_list_lock`: `j1939_tp_set_rxtimeout()` takes the lock and
  uses `hrtimer_try_to_cancel()` + `j1939_session_put()` on a return of 1
  before the get+start, and `j1939_session_cancel()` does the same under the
  lock it already takes. That is the pattern `j1939_cancel_active_session()`
  already uses. No caller of `j1939_tp_set_rxtimeout()` holds that lock today,
  so this does not create a new lock order.

That is a larger change than the current hunk, which is another reason to
split it out (see the subject finding below).

## Medium: WAITING_ABORT deactivation lets the ABORT echo hit the next session

Accepted, and this one is the more serious of the two. Confirmed in the tree:
`j1939_xtp_rx_abort()` calls `j1939_xtp_rx_abort_one()` twice, once with
`reverse = false` and once with `true`, and each does
`j1939_session_get_by_addr()`, which walks `active_session_list` and matches on
SA/DA/type/PGN only. Queued sessions are activated by
`j1939_sk_queue_activate_next_locked()`, which schedules the next session's
txtimer with `time_ms = 10 + get_random_u32_below(16)` when the previous
session carried an error, so there is a real 10-25 ms window in which B is on
the list and has not sent anything yet. An ABORT echo that is still in flight
from A then matches B, `j1939_xtp_rx_abort_one()` drops B with
`j1939_session_timers_cancel()` + `j1939_session_deactivate_activate_next()`,
and the sender of B gets an error for a transfer that never started.

The respin drops the `waiting` branch entirely and instead arms the abort
timer for both `J1939_SESSION_ACTIVE` and `J1939_SESSION_WAITING_ABORT`. Only
the `ACTIVE` case calls `__j1939_session_cancel()`, so a second error input
does not put a second ABORT frame on the bus; it just re-arms the timer. The
session stays on `active_session_list` either way. What the branch was working
around is that a second error input reaching the same session calls
`j1939_session_timers_cancel()` first, which cancels the abort timer armed by
the first call, and the second `j1939_session_cancel()` then found the state
already `J1939_SESSION_WAITING_ABORT`, matched neither arm of the old state
test, and armed nothing -- the stranding case from your subject finding, the
one that ends in `unregister_netdevice: waiting for vcan0 to become free`. With
the timer re-armed, the session either gets its own ABORT echo, which now still
finds it on the list because it was never deactivated, or `j1939_tp_rxtimer()`
takes the `J1939_SESSION_WAITING_ABORT` branch after
`J1939_XTP_ABORT_TIMEOUT_MS` and deactivates as before. Each re-arm stays
balanced: one `j1939_session_get()` against the one `j1939_session_put()` in
the callback.

## Low: the comment in `j1939_xtp_rx_dpo_one()` is wrong

Accepted, the comment is simply incorrect. `j1939_can_recv()` sets
`J1939_ECU_LOCAL_SRC` from the source address alone:

```c
	if (j1939_address_is_unicast(skcb->addr.sa) &&
	    priv->ents[skcb->addr.sa].nusers)
		skcb->flags |= J1939_ECU_LOCAL_SRC;
```

and `j1939_tp_cmd_recv()` forwards every ETP.CM_DPO with that flag to the
transmitter session, so any local node sending a bad DPO reaches the branch,
not only the echo of an EDPO this stack built. Both the comment and the
matching sentence in the commit message go away.

On the reason code: `J1939_XTP_ABORT_ECTS_TOO_BIG` ("ECTS requested packets
exceeds message size") is wrong for a DPO we received, so the check will send
`J1939_XTP_ABORT_BAD_EDPO_OFFSET` unconditionally. The transmitter-side case
that motivated the conditional is real, but its cause is not in this function:
`j1939_xtp_rx_cts_one()` bounds `pkt` only with `if (!pkt)` and
`dat[1] > session->pkt.block`, so an oversized ETP.CM_CTS packet number can set
`pkt.tx_acked` past `pkt.total`, and the EDPO built from it in
`j1939_session_tx_dpo()` carries that value back to us. The respin bounds the
CTS there -- `pkt > session->pkt.total` is exactly `ECTS_TOO_BIG` -- and then an
out-of-range DPO reaching the transmitter is a genuine peer error.

## Low: the subject covers only one of the changes

Accepted. v7 will be four patches:

1. `can: j1939: abort a session whose abort timer was cancelled` -- the
   WAITING_ABORT re-arm, with the two-unparsable-ETP.DT repro and the
   `ip link del` hang.
2. `can: j1939: arm the rx timer under the session list lock` -- the
   `j1939_tp_set_rxtimeout()` / `j1939_session_cancel()` serialization above.
3. `can: j1939: simplify the second RTS handling` -- the
   `j1939_xtp_rx_rts_session_active()` change.
4. `can: j1939: validate the EDPO packet number` -- the bound check, the NULL
   guard in `j1939_session_completed()`, and the CTS bound; the 256-packet
   repro and the 3/3 panic result stay with this one.

You are right that the `dpo >= pkt.total` check alone stops the T1 NULL
dereference; patches 1 and 2 are there for the reference strand, not for T1,
and I will say so in their messages instead of implying they are part of the
crash fix.

## Low: commit message and tags

Accepted, all three:

- The revision and testing history ("stock, v5, and this revision", the
  per-case matrix) moves below the `---` line.
- `From:` and `Signed-off-by:` will use the same name. The `Author:
  Quchaosheng` in v6 is a `git send-email` identity mismatch on my side, not
  an intentional alias.
- `Closes:` replaces `Link:` after `Reported-by:`.

Thanks for the review -- the two Medium findings in particular were worth the
round trip, and finding 3 saved a wrong abort code from going in.

Chaosheng

pw-bot: cr