[PATCH can-next v3 0/3] can: take rtnl_lock() before leaving the notifier list

[Oliver Hartkopp <socketcan@hartkopp.net>]

Norbert Szetei reported a use-after-free in isotp: isotp_release()
removes the socket from the netdev notifier list before it takes
rtnl_lock(). A socket that leaves the list in this window is skipped by
the notifier and has to remove its CAN filters itself. If the bound
device is moved to another network namespace in this window,
can_rx_unregister() silently skips the removal due to the namespace
mismatch and the remaining receivers point to the freed socket.

bcm has the same window and is affected in the same way. raw has the
window too, but removes its filters with dev_net(ro->dev) under RTNL
and is therefore not affected.

This series takes rtnl_lock() before the socket leaves the notifier
list in all three protocols. As the notifier chain is always invoked
under RTNL, the busy-wait loops on the *_busy_notifier variables are
dead code and are replaced with WARN_ON_ONCE().

For bcm, RTNL is only held for the notifier list removal and the
removal of the device specific filters. The procfs entry, the tx_ops
and the 'any' CAN device filters are removed after RTNL is released,
when bcm_notify() can not access the socket anymore.

Tested with PROVE_LOCKING, KASAN and DEBUG_ATOMIC_SLEEP while creating,
binding and releasing raw/bcm/isotp sockets on a vcan device that is
continuously moved between two network namespaces, set down and
re-created, with concurrent CAN traffic and bcm procfs readers.
A bcm procfs reader stalled via userfaultfd does not block RTNL while
bcm_release() waits for it in remove_proc_entry(), also not with a
concurrent removal of a CAN netdev in the socket's namespace.

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/

Changes in v3:
- bcm: remove the socket from the notifier list and the device specific
  filters under RTNL first. Then remove the procfs entry, the tx_ops and
  the 'any' CAN device filters without RTNL. In v2 a stalled procfs
  reader could still block bcm_notify() (under RTNL) on lock_sock()
  while bcm_release() waited in remove_proc_entry(), and the 'any' CAN
  device filters were removed under RTNL.
  Clearing bo->bcm_proc_read in bcm_release() is not needed anymore.
- bcm: replace the stale comment about a concurrent NETDEV_UNREGISTER
  in the rx_ops loop of bcm_release()

Changes in v2:
- bcm: remove the procfs entry and the tx_ops before taking RTNL so
  that remove_proc_entry() can not stall RTNL waiting for readers of
  the world-readable procfs entry
- bcm, raw: reword the commit messages so that they do not refer to
  patches outside of this series
- send all three patches as one series for can-next

Oliver Hartkopp (3):
  can: isotp: fix filter removal race between release and netns change
  can: bcm: fix filter removal race between release and netns change
  can: raw: take rtnl_lock() before leaving the notifier list

 net/can/bcm.c   | 47 +++++++++++++++++++++--------------------------
 net/can/isotp.c | 17 +++++++++--------
 net/can/raw.c   | 13 +++++++------
 3 files changed, 37 insertions(+), 40 deletions(-)

-- 
2.43.0