[RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device

[RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device

[Oleksij Rempel]

Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
ml_priv") the CAN framework uses per device specific data in the AF_CAN
protocol. For this purpose the struct net_device->ml_priv is used. Later
the ml_priv usage in CAN was extended for other users, one of them being
CAN_J1939.

Later in the kernel ml_priv was converted to an union, used by other
drivers. E.g. the tun driver started storing it's stats pointer.

Since tun devices can claim to be a CAN device, CAN specific protocols
will wrongly interpret this pointer, which will cause system crashes.
Mostly this issue is visible in the CAN_J1939 stack.

To fix this issue, we request a dedicated CAN pointer within the
net_device struct.

Reported-by: syzbot+5138c4dd15a0401bec7b@syzkaller.appspotmail.com
Fixes: 20dd3850bcf8 ("can: Speed up CAN frame receiption by using ml_priv")
Fixes: ffd956eef69b ("can: introduce CAN midlayer private and allocate it automatically")
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Fixes: 497a5757ce4e ("tun: switch to net core provided statistics counters")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/can/dev/dev.c |  2 +-
 drivers/net/can/slcan.c   |  2 +-
 drivers/net/can/vcan.c    |  2 +-
 drivers/net/can/vxcan.c   |  2 +-
 include/linux/netdevice.h |  4 ++++
 net/can/af_can.c          |  4 ++--
 net/can/j1939/main.c      |  4 ++--
 net/can/j1939/socket.c    |  2 +-
 net/can/proc.c            | 13 +++++++------
 9 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index f580f0ac6497..0e5265cb635f 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -269,7 +269,7 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 	priv = netdev_priv(dev);
 	priv->dev = dev;
 
-	dev->ml_priv = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN);
+	dev->can = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN);
 
 	if (echo_skb_max) {
 		priv->echo_skb_max = echo_skb_max;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index a1bd1be09548..f703bd3b7415 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -538,7 +538,7 @@ static struct slcan *slc_alloc(void)
 
 	dev->base_addr  = i;
 	sl = netdev_priv(dev);
-	dev->ml_priv = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
+	dev->can = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
 
 	/* Initialize channel control data */
 	sl->magic = SLCAN_MAGIC;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 39ca14b0585d..d8c8f9cc769f 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -153,7 +153,7 @@ static void vcan_setup(struct net_device *dev)
 	dev->addr_len		= 0;
 	dev->tx_queue_len	= 0;
 	dev->flags		= IFF_NOARP;
-	dev->ml_priv		= netdev_priv(dev);
+	dev->can		= netdev_priv(dev);
 
 	/* set flags according to driver capabilities */
 	if (echo)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index fa47bab510bb..0fea82935bf0 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -147,7 +147,7 @@ static void vxcan_setup(struct net_device *dev)
 	dev->flags		= (IFF_NOARP|IFF_ECHO);
 	dev->netdev_ops		= &vxcan_netdev_ops;
 	dev->needs_free_netdev	= true;
-	dev->ml_priv		= netdev_priv(dev) + ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN);
+	dev->can		= netdev_priv(dev) + ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN);
 }
 
 /* forward declaration for rtnl_create_link() */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5b949076ed23..825220da2e4f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2153,6 +2153,10 @@ struct net_device {
 
 	/* protected by rtnl_lock */
 	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
+
+#if IS_ENABLED(CONFIG_CAN)
+	struct can_ml_priv	*can;
+#endif
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 837bb8af0ec3..8651c8112a65 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -304,7 +304,7 @@ static struct can_dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
 							struct net_device *dev)
 {
 	if (dev) {
-		struct can_ml_priv *ml_priv = dev->ml_priv;
+		struct can_ml_priv *ml_priv = dev->can;
 		return &ml_priv->dev_rcv_lists;
 	} else {
 		return net->can.rx_alldev_list;
@@ -801,7 +801,7 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 
 	switch (msg) {
 	case NETDEV_REGISTER:
-		WARN(!dev->ml_priv,
+		WARN(!dev->can,
 		     "No CAN mid layer private allocated, please fix your driver and use alloc_candev()!\n");
 		break;
 	}
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index bb914d8b4216..62088074230d 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -140,7 +140,7 @@ static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
 static inline void j1939_priv_set(struct net_device *ndev,
 				  struct j1939_priv *priv)
 {
-	struct can_ml_priv *can_ml_priv = ndev->ml_priv;
+	struct can_ml_priv *can_ml_priv = ndev->can;
 
 	can_ml_priv->j1939_priv = priv;
 }
@@ -211,7 +211,7 @@ static void __j1939_rx_release(struct kref *kref)
 /* get pointer to priv without increasing ref counter */
 static inline struct j1939_priv *j1939_ndev_to_priv(struct net_device *ndev)
 {
-	struct can_ml_priv *can_ml_priv = ndev->ml_priv;
+	struct can_ml_priv *can_ml_priv = ndev->can;
 
 	if (!can_ml_priv)
 		return NULL;
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index f23966526a88..8010fbc8bd29 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -467,7 +467,7 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 			goto out_release_sock;
 		}
 
-		if (!ndev->ml_priv) {
+		if (!ndev->can) {
 			netdev_warn_once(ndev,
 					 "No CAN mid layer private allocated, please fix your driver and use alloc_candev()!\n");
 			dev_put(ndev);
diff --git a/net/can/proc.c b/net/can/proc.c
index 5ea8695f507e..7d7a7f6d2cc9 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -322,8 +322,9 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
 
 	/* receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
-		if (dev->type == ARPHRD_CAN && dev->ml_priv)
-			can_rcvlist_proc_show_one(m, idx, dev, dev->ml_priv);
+		if (dev->type == ARPHRD_CAN && dev->can)
+			can_rcvlist_proc_show_one(m, idx, dev,
+						  &dev->can->dev_rcv_lists);
 	}
 
 	rcu_read_unlock();
@@ -375,8 +376,8 @@ static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
 
 	/* sff receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
-		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			dev_rcv_lists = dev->ml_priv;
+		if (dev->type == ARPHRD_CAN && dev->can) {
+			dev_rcv_lists = &dev->can->dev_rcv_lists;
 			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_sff,
 						    ARRAY_SIZE(dev_rcv_lists->rx_sff));
 		}
@@ -406,8 +407,8 @@ static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
 
 	/* eff receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
-		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			dev_rcv_lists = dev->ml_priv;
+		if (dev->type == ARPHRD_CAN && dev->can) {
+			dev_rcv_lists = &dev->can->dev_rcv_lists;
 			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_eff,
 						    ARRAY_SIZE(dev_rcv_lists->rx_eff));
 		}
-- 
2.30.0

[RFC PATCH net 2/2] net: can: j1939: fix check for valid CAN devices

[Oleksij Rempel]

With the last patch a dedicated struct can_ml pointer was added to the
struct netdevice to store CAN stack related private data. The data is
only allocated and the pointer is only set by CAN devices.

Now we use a NULL pointer check on ndev->can to check for real CAN
devices. Only checking the ARPHRD via ndev->type is not sufficient,
since it can be set by user space to an arbitrary value for tun/tap
devices.

Since the ndev->type and ndev->can are now checked early, this patch
removes obsolete checks further down the call stacks.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/main.c   | 12 +++---------
 net/can/j1939/socket.c |  2 +-
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 62088074230d..fbc0d25046e2 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -213,9 +213,6 @@ static inline struct j1939_priv *j1939_ndev_to_priv(struct net_device *ndev)
 {
 	struct can_ml_priv *can_ml_priv = ndev->can;
 
-	if (!can_ml_priv)
-		return NULL;
-
 	return can_ml_priv->j1939_priv;
 }
 
@@ -225,9 +222,6 @@ static struct j1939_priv *j1939_priv_get_by_ndev_locked(struct net_device *ndev)
 
 	lockdep_assert_held(&j1939_netdev_lock);
 
-	if (ndev->type != ARPHRD_CAN)
-		return NULL;
-
 	priv = j1939_ndev_to_priv(ndev);
 	if (priv)
 		j1939_priv_get(priv);
@@ -350,13 +344,13 @@ static int j1939_netdev_notify(struct notifier_block *nb,
 	struct net_device *ndev = netdev_notifier_info_to_dev(data);
 	struct j1939_priv *priv;
 
+	if (ndev->type != ARPHRD_CAN || !ndev->can)
+		goto notify_put;
+
 	priv = j1939_priv_get_by_ndev(ndev);
 	if (!priv)
 		goto notify_done;
 
-	if (ndev->type != ARPHRD_CAN)
-		goto notify_put;
-
 	switch (msg) {
 	case NETDEV_DOWN:
 		j1939_cancel_active_session(priv, NULL);
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 8010fbc8bd29..61732e558980 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -461,7 +461,7 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 			goto out_release_sock;
 		}
 
-		if (ndev->type != ARPHRD_CAN) {
+		if (ndev->type != ARPHRD_CAN || !ndev->can) {
 			dev_put(ndev);
 			ret = -ENODEV;
 			goto out_release_sock;
-- 
2.30.0

Re: [RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device

[Jakub Kicinski]

On Fri, 15 Jan 2021 15:30:35 +0100 Oleksij Rempel wrote:
> Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
> ml_priv") the CAN framework uses per device specific data in the AF_CAN
> protocol. For this purpose the struct net_device->ml_priv is used. Later
> the ml_priv usage in CAN was extended for other users, one of them being
> CAN_J1939.
> 
> Later in the kernel ml_priv was converted to an union, used by other
> drivers. E.g. the tun driver started storing it's stats pointer.
> 
> Since tun devices can claim to be a CAN device, CAN specific protocols
> will wrongly interpret this pointer, which will cause system crashes.
> Mostly this issue is visible in the CAN_J1939 stack.
> 
> To fix this issue, we request a dedicated CAN pointer within the
> net_device struct.

No strong objection, others already added their pointers, but 
I wonder if we can't save those couple of bytes by adding a
ml_priv_type, to check instead of dev->type? And leave it 0
for drivers using stats?

That way other device types which are limited by all being 
ARPHDR_ETHER can start using ml_priv as well?

> +#if IS_ENABLED(CONFIG_CAN)
> +	struct can_ml_priv	*can;
> +#endif

Perhaps put it next to all the _ptr fields?

Re: [RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

On Fri, Jan 15, 2021 at 04:07:23PM -0800, Jakub Kicinski wrote:
> On Fri, 15 Jan 2021 15:30:35 +0100 Oleksij Rempel wrote:
> > Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
> > ml_priv") the CAN framework uses per device specific data in the AF_CAN
> > protocol. For this purpose the struct net_device->ml_priv is used. Later
> > the ml_priv usage in CAN was extended for other users, one of them being
> > CAN_J1939.
> > 
> > Later in the kernel ml_priv was converted to an union, used by other
> > drivers. E.g. the tun driver started storing it's stats pointer.
> > 
> > Since tun devices can claim to be a CAN device, CAN specific protocols
> > will wrongly interpret this pointer, which will cause system crashes.
> > Mostly this issue is visible in the CAN_J1939 stack.
> > 
> > To fix this issue, we request a dedicated CAN pointer within the
> > net_device struct.
> 
> No strong objection, others already added their pointers, but 
> I wonder if we can't save those couple of bytes by adding a
> ml_priv_type, to check instead of dev->type? And leave it 0
> for drivers using stats?

Sounds good.

If we want to save even more bytes, it might be possible, to move the
wireless and wpan pointers to ml_priv.

	struct wireless_dev	*ieee80211_ptr;
	struct wpan_dev		*ieee802154_ptr;

> That way other device types which are limited by all being 
> ARPHDR_ETHER can start using ml_priv as well?
> 
> > +#if IS_ENABLED(CONFIG_CAN)
> > +	struct can_ml_priv	*can;
> > +#endif
> 
> Perhaps put it next to all the _ptr fields?

Makes sense. Oleksij will resping the series.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]