linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v13 0/6] can: j1939: make module more robust
@ 2026-11-15  5:34 Tetsuo Handa
  2026-11-15  5:34 ` [PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload Tetsuo Handa
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Tetsuo Handa @ 2026-11-15  5:34 UTC (permalink / raw)
  To: linux-can, Marc Kleine-Budde, Oleksij Rempel,
	Robin van der Gracht, kernel, Oliver Hartkopp

This series is intended to address the following problems.

## Problem 1: Possible UAF on module unload.

Since module refcount is currently associated with lifecycle of j1939
sockets, we need to make sure that all resources associated with socket
("struct j1939_priv" and "struct j1939_ecu") are synchronously destructed
before socket is destructed. Otherwise, j1939_module_exit() can complete
and memory for this module is unmapped before j1939_ecu_timer_handler() or
__j1939_ecu_release() or __j1939_priv_release() completes (i.e. causing
UAF).

Calling module_put(THIS_MODULE) from a module function which is not
possible to wait for completion of that function from that module's exit
function is a source of UAF. But calling module_put(THIS_MODULE) from a WQ
function which is possible to wait for completion from that module's exit
function using flush_workqueue() etc. is fine.

Fix problem 1 by deferring release steps of "struct j1939_priv" to WQ
context where we can call module_put(THIS_MODULE) while allowing
j1939_module_exit() to wait for completion using destroy_workqueue().

By this change, we no longer need to make sure that all resources
associated with socket ("struct j1939_priv" and "struct j1939_ecu") are
synchronously destructed before socket is destructed, simplifying
further fixes.

## Problem 2: Possible UAF in __j1939_priv_release().

If a user calls j1939_ecu_put(ecu) with priv->lock held but without knowing
that this "struct j1939_ecu" is the last user of a "struct j1939_priv",
__j1939_priv_release_work() via __j1939_priv_release() from
j1939_priv_put() from __j1939_ecu_release() might call kfree(priv)
before priv->lock is released (i.e. causing UAF).

Fix problem 2 by adding a safeguard that delays kfree() in
__j1939_priv_release_work() until there is no priv->lock holder.

## Problem 3: Possible list corruption in __j1939_ecu_release().

Since j1939_ac_recv() and j1939_ac_fixup() are calling j1939_ecu_put()
without priv->lock held for write, list corruption might happen if the
last user called j1939_ecu_put() without priv->lock held for write.

Instead of requiring all j1939_ecu_put() callers to hold priv->lock for
write, fix problem 3 by deferring release steps of "struct j1939_ecu"
to WQ context where we can unconditionally hold priv->lock for write.

By deferring to WQ context, readers might find ECUs which have already
reached __j1939_ecu_release(). Therefore, allow using an ECU only when
__j1939_ecu_release() is not yet called (indicated by ecu->kref > 0).

## Problem 4: Possible UAF in j1939_ecu_timer_handler().

Since pending ecu->ac_timer is not canceled during j1939_ecu_unmap_all()
or __j1939_rx_release(), ecu->ac_timer can call j1939_ecu_timer_handler()
after __j1939_ecu_release() or j1939_module_exit() (i.e. causing UAF).

Fix problem 4 by adding a safeguard that explicitly cancels ecu->ac_timer
before kfree(). The fix for problem 3 makes it possible to call
hrtimer_cancel() from __j1939_ecu_release_work().

## Problem 5: Possible "struct j1939_ecu" refcount leak.

Since

  j1939_ecu_get(ecu);
  priv->ents[ecu->addr] = ecu;

in j1939_ecu_map_locked() from j1939_ecu_timer_handler() can succeed
even after

  priv->ents[ecu->addr] = NULL;
  j1939_ecu_put(ecu);

in j1939_ecu_unmap_locked() from j1939_ecu_unmap_all() from
__j1939_rx_release() from j1939_netdev_stop() has completed,
"struct j1939_ecu" refcount leaks.

Fix problem 5 by

  (1) preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start()
  (2) making j1939_ecu_map_locked() no-op

when a netdev which is associated with j1939_priv is no longer registered
(indicated by ecu->priv->ndev->reg_state != NETREG_REGISTERED) or
__j1939_rx_release() was called (indicated by ecu->priv->rx_kref == 0).

## Problem 6: Possible hard-lockup/deadlock in j1939_ecu_timer_cancel().

Since j1939_ecu_timer_cancel() is calling hrtimer_cancel() with priv->lock
held for write while j1939_ecu_timer_handler() takes priv->lock, a hard-
lockup (if CONFIG_PREEMPT_RT=n && CONFIG_SMP=y) or a deadlock between
priv->lock and cpu_base->softirq_expiry_lock (if CONFIG_PREEMPT_RT=y) will
happen when hrtimer_cancel() calls hrtimer_cancel_wait_running().

The comment for "struct j1939_priv"->lock says that priv->lock is used for
protecting "struct list_head ecus" and "struct j1939_addr_ent ents[256]".
But j1939_ac_process() is using priv->lock for also serializing timer
management across multiple frames. This makes it impossible to temporarily
drop priv->lock in order to safely call hrtimer_cancel().

Therefore, fix problem 6 by introducing two bool flags that implement
logical cancellation (negotiate hrtimer_try_to_cancel() == -1 condition)
and dropping address claim frames during logical cancellation.



[PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload
[PATCH v13 2/6] can: j1939: add a safeguard to __j1939_priv_release_work()
[PATCH v13 3/6] can: j1939: fix possibility of list corruption in __j1939_ecu_release()
[PATCH v13 4/6] can: j1939: add a safeguard to __j1939_ecu_release_work()
[PATCH v13 5/6] can: j1939: fix "struct j1939_ecu" refcount leak
[PATCH v13 6/6] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()
---
Changes since v12:
- Added a cover letter and split the series into 6 logical patches (1 patch per problem).
- Deferred __j1939_priv_release() to WQ context to simplify "struct j1939_ecu" refcount leak fix
- Fixed findings by sashiko in v12.

 net/can/j1939/address-claim.c |    7 +++-
 net/can/j1939/bus.c           |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 net/can/j1939/j1939-priv.h    |   12 +++++++-
 net/can/j1939/main.c          |   75 ++++++++++++++++++++++++++++++++++++++++++++------
 net/can/j1939/socket.c        |   10 ++++++
 5 files changed, 189 insertions(+), 36 deletions(-)

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-11-15  6:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-15  5:34 [PATCH v13 0/6] can: j1939: make module more robust Tetsuo Handa
2026-11-15  5:34 ` [PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload Tetsuo Handa
2026-11-15  6:01   ` sashiko-bot
2026-11-15  5:34 ` [PATCH v13 2/6] can: j1939: add a safeguard to __j1939_priv_release_work() Tetsuo Handa
2026-11-15  5:35 ` [PATCH v13 3/6] can: j1939: fix possibility of list corruption in __j1939_ecu_release() Tetsuo Handa
2026-11-15  5:35 ` [PATCH v13 4/6] can: j1939: add a safeguard to __j1939_ecu_release_work() Tetsuo Handa
2026-11-15  5:35 ` [PATCH v13 5/6] can: j1939: fix "struct j1939_ecu" refcount leak Tetsuo Handa
2026-11-15  5:36 ` [PATCH v13 6/6] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel() Tetsuo Handa
2026-11-15  5:51   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).