Re: [PATCH v5 2/3] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not say what user-visible bug this
  stable-tagged patch fixes.

--

can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

This patch replaces the dev->type != ARPHRD_CAN checks in af_can, bcm, gw,
isotp and raw with can_get_ml_priv() checks. The stated reason is that
TUNSETLINK lets a down TUN/TAP device change its type to ARPHRD_CAN while
dev->ml_priv stays NULL.

> This has been used directly by the referenced commit in the CAN specific
> j1939 and proc code but not in the other parts of the CAN subsystem.

[Severity: Low]
Is the "not in the other parts of the CAN subsystem" part accurate?

af_can.c already checks can_get_ml_priv() in can_rx_register(), which was
added by commit 8aa59e355949 ("can: af_can: fix NULL pointer dereference
in can_rx_register()"). It is also checked in can_rcv(), canfd_rcv() and
canxl_rcv(). The removed lines in this diff show it:

    if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
        return -ENODEV;

> Instead of checking dev->type being the unreliable ARPHRD_CAN value convert
> the missing "valid CAN devices" checks to can_get_ml_priv().

The patch is tagged for stable, but the message reads like a cleanup.
Could it also describe the user-visible bug being fixed?

Before this patch, a NULL pointer dereference looks reachable through
CAN_ISOTP. isotp_bind() only checked dev->type, so it accepted a TUN
device whose type had been set to ARPHRD_CAN. It also ignores the
-ENODEV that can_rx_register() returns:

    if (isotp_register_rxid(so))
        can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
                        isotp_rcv, sk, "isotp", sk);
    ...
    so->bound = 1;
    ...
    so->dev = dev;

On close(), isotp_release() then calls can_rx_unregister() on so->dev.
Before this patch, can_rx_unregister() only checked the type:

    if (dev && dev->type != ARPHRD_CAN)
        return;

So it carried on into can_dev_rcv_lists_find(), which has no NULL check:

    struct can_ml_priv *can_ml = can_get_ml_priv(dev);
    return &can_ml->dev_rcv_lists;

dev_rcv_lists is the first member of struct can_ml_priv, so this returns
NULL. The call chain is:

close()
  isotp_release()
    can_rx_unregister()
      can_dev_rcv_lists_find()     <- returns NULL
      can_rcv_list_find()
      hlist_for_each_entry_rcu()   <- NULL dereference

isotp_notify() for NETDEV_UNREGISTER reaches the same path. Creating the
TUN device needs CAP_NET_ADMIN, and an unprivileged user can get that
inside a user namespace.

Could the message mention the crash, the affected protocol, and the
trigger (TUNSETLINK to ARPHRD_CAN, then binding a CAN_ISOTP socket)? That
would make it clear to stable backporters that this is a crash fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929163424.16382-1-socketcan%40hartkopp.net