Re: [PATCH net-next 03/37] 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 calls the change a conversion of 'unreliable'
  ARPHRD_CAN checks to 'robust' can_get_ml_priv() checks.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but isotp_bind()
  in net/can/isotp.c ignores the return values of both can_rx_register()…

--

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

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

Link: https://patch.msgid.link/20261009121115.61012-1-socketcan@hartkopp.net

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 34fe3b28d576f..dc27ace43719d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c

[ ... ]

> @@ -519,7 +519,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>  	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
>  	struct can_dev_rcv_lists *dev_rcv_lists;
>  
> -	if (dev && dev->type != ARPHRD_CAN)
> +	if (dev && !can_get_ml_priv(dev))
>  		return;

[Severity: Low]
The commit message describes this change as replacing "unreliable" checks
with "robust" ones. Should it also say that the change closes a NULL
pointer dereference that a user can trigger?

Before this patch, the sequence looks like this:

  ioctl(TUNSETLINK, ARPHRD_CAN) on a down tun device
    tun->dev->type = (int) arg;      /* ml_priv stays NULL */
  isotp_bind()
    old dev->type != ARPHRD_CAN check passes
    can_rx_register() returns -ENODEV, return value ignored
    so->bound = 1; so->dev = dev;
  isotp_release() or isotp_notify()
    can_rx_unregister(net, so->dev, ...)
      old dev->type != ARPHRD_CAN guard passes
      can_dev_rcv_lists_find()
        can_ml = can_get_ml_priv(dev);   /* NULL */
        return &can_ml->dev_rcv_lists;
      hlist_for_each_entry_rcu() walks the near-NULL pointer

dev_rcv_lists sits at offset 0 of struct can_ml_priv, so the returned
pointer is near-NULL and the list walk oopses.

Triggering this only requires owning a tun device, or CAP_NET_ADMIN inside
an unprivileged user namespace.

The patch carries a Cc: stable tag. Could the message name the oops in
can_rx_unregister() (reached through isotp_release() or isotp_notify()) and
the privilege needed to trigger it?

[ ... ]

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index f5dc9d04bd686..6c28802c0605f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1610,7 +1611,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		err = -ENODEV;
>  		goto out;
>  	}
> -	if (dev->type != ARPHRD_CAN) {
> +	if (!can_get_ml_priv(dev)) {
>  		err = -ENODEV;
>  		goto out_put_dev;
>  	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Further
down, isotp_bind() still ignores the return values of both
can_rx_register() calls:

	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, ...);
	...
	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
			isotp_rcv_echo, sk, ...);

	/* switch to new settings */
	...
	so->bound = 1;

The can_get_ml_priv() check above now rules out -ENODEV. Using
dev_get_by_index(net, ...) also guarantees the netns matches. That leaves
one possible failure: -ENOMEM from can_rx_register():

	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
	if (!rcv)
		return -ENOMEM;

If that allocation fails, bind() still returns 0. The socket then never
receives flow control frames, or never receives its own TX echoes.

Transfers would stall until the hrtimers fire. Later, isotp_release() or
isotp_notify() would hit the "receive list entry not found" warning in
can_rx_unregister().

Should isotp_bind() check these return values, and unwind the first
registration if the second one fails?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009134323.64064-2-mkl%40pengutronix.de