pull-request: can 2021-01-20

Hello Jakub, hello David,

this is a pull request of 3 patches for net/master.

All three patches are by Vincent Mailhol and fix a potential use after free bug
in the CAN device infrastructure, the vxcan driver, and the peak_usk driver. In
the TX-path the skb is used to read from after it was passed to the networking
stack with netif_rx_ni().

Note: Patch 1/3 touches "drivers/net/can/dev.c". In net-next/master this file
has been moved to drivers/net/can/dev/dev.c [1] and parts of it have been
transfered into separate files. This may result in a merge conflict. Please
carry this patch forward, the change is rather simple. Drop us a note if
needed. Are any actions needed with regards to linux-next?

[1] 3e77f70e7345 can: dev: move driver related infrastructure into separate subdir

regards,
Marc

---

The following changes since commit 9c30ae8398b0813e237bde387d67a7f74ab2db2d:

  tcp: fix TCP socket rehash stats mis-accounting (2021-01-19 19:47:20 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-5.11-20210120

for you to fetch changes up to 50aca891d7a554db0901b245167cd653d73aaa71:

  can: peak_usb: fix use after free bugs (2021-01-20 13:33:28 +0100)

----------------------------------------------------------------
linux-can-fixes-for-5.11-20210120

----------------------------------------------------------------
Vincent Mailhol (3):
      can: dev: can_restart: fix use after free bug
      can: vxcan: vxcan_xmit: fix use after free bug
      can: peak_usb: fix use after free bugs

 drivers/net/can/dev.c                      | 4 ++--
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 8 ++++----
 drivers/net/can/vxcan.c                    | 6 ++++--
 3 files changed, 10 insertions(+), 8 deletions(-)

From: Vincent Mailhol <redacted>

After calling netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the can_frame cf which aliases skb memory is accessed
after the netif_rx_ni() in:
      stats->rx_bytes += cf->len;

Reordering the lines solves the issue.

Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Link: https://lore.kernel.org/r/20210120114137.200019-2-mailhol.vincent@wanadoo.fr
Signed-off-by: Vincent Mailhol <redacted>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3486704c8a95..8b1ae023cb21 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c

@@ -592,11 +592,11 @@ static void can_restart(struct net_device *dev)
 
 	cf->can_id |= CAN_ERR_RESTARTED;
 
-	netif_rx_ni(skb);
-
 	stats->rx_packets++;
 	stats->rx_bytes += cf->len;
 
+	netif_rx_ni(skb);
+
 restart:
 	netdev_dbg(dev, "restarted\n");
 	priv->can_stats.restarts++;

base-commit: 9c30ae8398b0813e237bde387d67a7f74ab2db2d
-- 
2.29.2

On Wed, 20 Jan 2021 13:51:59 +0100 Marc Kleine-Budde wrote:

Hello Jakub, hello David,

this is a pull request of 3 patches for net/master.

All three patches are by Vincent Mailhol and fix a potential use after free bug
in the CAN device infrastructure, the vxcan driver, and the peak_usk driver. In
the TX-path the skb is used to read from after it was passed to the networking
stack with netif_rx_ni().

Pulled, thanks.

Seems like the PR didn't show up in patchwork at all :S Hopefully I can
still pull reight manually without the scripts :)

Note: Patch 1/3 touches "drivers/net/can/dev.c". In net-next/master this file
has been moved to drivers/net/can/dev/dev.c [1] and parts of it have been
transfered into separate files. This may result in a merge conflict. Please
carry this patch forward, the change is rather simple. Drop us a note if
needed. Are any actions needed with regards to linux-next?

Thanks for the note, I'm sending the PR to Linus now, so I think
linux-next may never see the the conflict.

On 1/20/21 6:19 PM, Jakub Kicinski wrote:

quoted

this is a pull request of 3 patches for net/master.

All three patches are by Vincent Mailhol and fix a potential use after free bug
in the CAN device infrastructure, the vxcan driver, and the peak_usk driver. In
the TX-path the skb is used to read from after it was passed to the networking
stack with netif_rx_ni().

Pulled, thanks.

Seems like the PR didn't show up in patchwork at all :S Hopefully I can
still pull reight manually without the scripts :)

Fingers crossed. :D

Today I noticed a lag of >4h on vger.kernel.org. Even this mail of yours hasn't
made it to the linux-can list, yet. It's 3h delayed.

quoted

Note: Patch 1/3 touches "drivers/net/can/dev.c". In net-next/master this file
has been moved to drivers/net/can/dev/dev.c [1] and parts of it have been
transfered into separate files. This may result in a merge conflict. Please
carry this patch forward, the change is rather simple. Drop us a note if
needed. Are any actions needed with regards to linux-next?

Thanks for the note, I'm sending the PR to Linus now, so I think
linux-next may never see the the conflict.

thanks,
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 |

From: Vincent Mailhol <redacted>

After calling peak_usb_netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the can_frame cf which aliases skb memory is accessed
after the peak_usb_netif_rx_ni().

Reordering the lines solves the issue.

Fixes: 0a25e1f4f185 ("can: peak_usb: add support for PEAK new CANFD USB adapters")
Link: https://lore.kernel.org/r/20210120114137.200019-4-mailhol.vincent@wanadoo.fr
Signed-off-by: Vincent Mailhol <redacted>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 61631f4fd92a..f347ecc79aef 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c

@@ -514,11 +514,11 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
 	else
 		memcpy(cfd->data, rm->d, cfd->len);
 
-	peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(rm->ts_low));
-
 	netdev->stats.rx_packets++;
 	netdev->stats.rx_bytes += cfd->len;
 
+	peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(rm->ts_low));
+
 	return 0;
 }
 

@@ -580,11 +580,11 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
 	if (!skb)
 		return -ENOMEM;
 
-	peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(sm->ts_low));
-
 	netdev->stats.rx_packets++;
 	netdev->stats.rx_bytes += cf->len;
 
+	peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(sm->ts_low));
+
 	return 0;
 }
 

-- 
2.29.2

On Wed, 20 Jan 2021 21:20:13 +0100 Marc Kleine-Budde wrote:

On 1/20/21 6:19 PM, Jakub Kicinski wrote:

quoted

quoted

this is a pull request of 3 patches for net/master.

All three patches are by Vincent Mailhol and fix a potential use after free bug
in the CAN device infrastructure, the vxcan driver, and the peak_usk driver. In
the TX-path the skb is used to read from after it was passed to the networking
stack with netif_rx_ni().  

Pulled, thanks.

Seems like the PR didn't show up in patchwork at all :S Hopefully I can
still pull reight manually without the scripts :)  

Fingers crossed. :D

Today I noticed a lag of >4h on vger.kernel.org. Even this mail of yours hasn't
made it to the linux-can list, yet. It's 3h delayed.

It's been reported but it's unclear what's causing this one :(

quoted

quoted

Note: Patch 1/3 touches "drivers/net/can/dev.c". In net-next/master this file
has been moved to drivers/net/can/dev/dev.c [1] and parts of it have been
transfered into separate files. This may result in a merge conflict. Please
carry this patch forward, the change is rather simple. Drop us a note if
needed. Are any actions needed with regards to linux-next?  

Thanks for the note, I'm sending the PR to Linus now, so I think
linux-next may never see the the conflict.  

The merge has been done now, could you double check?

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Wed, 20 Jan 2021 13:52:00 +0100 you wrote:

From: Vincent Mailhol <redacted>

After calling netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the can_frame cf which aliases skb memory is accessed
after the netif_rx_ni() in:
      stats->rx_bytes += cf->len;

[...]

Here is the summary with links:
  - [net,1/3] can: dev: can_restart: fix use after free bug
    https://git.kernel.org/netdev/net-next/c/03f16c5075b2
  - [net,2/3] can: vxcan: vxcan_xmit: fix use after free bug
    https://git.kernel.org/netdev/net-next/c/75854cad5d80
  - [net,3/3] can: peak_usb: fix use after free bugs
    https://git.kernel.org/netdev/net-next/c/50aca891d7a5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Hello:

This pull request was applied to netdev/net-next.git (refs/heads/master):

On Wed, 20 Jan 2021 13:51:59 +0100 you wrote:

Hello Jakub, hello David,

this is a pull request of 3 patches for net/master.

All three patches are by Vincent Mailhol and fix a potential use after free bug
in the CAN device infrastructure, the vxcan driver, and the peak_usk driver. In
the TX-path the skb is used to read from after it was passed to the networking
stack with netif_rx_ni().

[...]

Here is the summary with links:
  - pull-request: can 2021-01-20
    https://git.kernel.org/netdev/net-next/c/535d31593f59

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

From: Vincent Mailhol <redacted>

After calling netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the canfd_frame cfd which aliases skb memory is accessed
after the netif_rx_ni().

Fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
Link: https://lore.kernel.org/r/20210120114137.200019-3-mailhol.vincent@wanadoo.fr
Signed-off-by: Vincent Mailhol <redacted>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/vxcan.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index fa47bab510bb..f9a524c5f6d6 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c

@@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct net_device *peer;
 	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *peerstats, *srcstats = &dev->stats;
+	u8 len;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;

@@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb->dev        = peer;
 	skb->ip_summed  = CHECKSUM_UNNECESSARY;
 
+	len = cfd->len;
 	if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
 		srcstats->tx_packets++;
-		srcstats->tx_bytes += cfd->len;
+		srcstats->tx_bytes += len;
 		peerstats = &peer->stats;
 		peerstats->rx_packets++;
-		peerstats->rx_bytes += cfd->len;
+		peerstats->rx_bytes += len;
 	}
 
 out_unlock:

-- 
2.29.2

On 1/20/21 9:36 PM, Jakub Kicinski wrote:

quoted

quoted

quoted

Note: Patch 1/3 touches "drivers/net/can/dev.c". In net-next/master this file
has been moved to drivers/net/can/dev/dev.c [1] and parts of it have been
transfered into separate files. This may result in a merge conflict. Please
carry this patch forward, the change is rather simple. Drop us a note if
needed. Are any actions needed with regards to linux-next?  

Thanks for the note, I'm sending the PR to Linus now, so I think
linux-next may never see the the conflict.  

The merge has been done now, could you double check?

Looks good!

Thanks,
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 |