Skip to contentSkip to conversation

can: restore skb header initialisations in init_can_skb()

PATCH Superseded zjamg 2 participants 2 messages Sep 17

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

zjamg

author
#0 lore raw

PATCH 1/1 can: restore skb header initialisations in init_can_skb()

Commit 9f10374bb024 ("can: remove private CAN skb headroom infrastructure") removed the skb_reset_mac_header()/skb_reset_network_header()/ skb_reset_transport_header() calls from init_can_skb(). As a result, RX skbs from alloc_can_skb() and friends again carry mac_header = 0xFFFF. When such an skb reaches packet_rcv_spkt() (SOCK_PACKET), the push length calculation overflows and triggers skb_under_panic -> kernel BUG -> full machine panic.

The same issue was originally reported in 2014 on linux-can and fixed by commit 969439016d2c ("can: add missing initialisations in CAN related skbuffs"). packet_rcv_spkt() itself has never been hardened: only packet_rcv() and tpacket_rcv() gained dev_has_header() checks in commit d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header").

Fixes 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
---
 drivers/net/can/dev/skb.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/can/dev/skb.c +8 -0
index 95fcdc1026f8..a0712e607b28 100644
@@ -210,6 +210,14 @@ static void init_can_skb(struct sk_buff *skb)
 {
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	/* Restore the initialisations that were present before
+	 * 9f10374bb024 to prevent skb_under_panic in packet_rcv_spkt
+	 * and similar consumers.
+	 */
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
 }
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
Signature · 1 line
2.53.0
Reviewed-by Acked-by on v1 1/1
#1 lore raw

Hello zjamg!

Many thanks for the patch!

Quoted 8 lines · zjamg

Commit 9f10374bb024 ("can: remove private CAN skb headroom infrastructure") removed the skb_reset_mac_header()/skb_reset_network_header()/ skb_reset_transport_header() calls from init_can_skb(). As a result, RX skbs from alloc_can_skb() and friends again carry mac_header = 0xFFFF. When such an skb reaches packet_rcv_spkt() (SOCK_PACKET), the push length calculation overflows and triggers skb_under_panic -> kernel BUG -> full machine panic.

Quoted 6 lines

The same issue was originally reported in 2014 on linux-can and fixed by commit 969439016d2c ("can: add missing initialisations in CAN related skbuffs"). packet_rcv_spkt() itself has never been hardened: only packet_rcv() and tpacket_rcv() gained dev_has_header() checks in commit d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header").

Right. My bad.

I looked into netdev_alloc_skb() to see the the values were initialized and therefore thought I can leave it out.

But there are initialized in a way that the header settings do not present valid settings, e.g. mac_header = 0xFFFF

So the functions definitely need to be called as you correctly remarked.

Quoted 3 lines
Fixes 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")

You can add my

in your v2 patch.

Quoted 9 lines
---
  drivers/net/can/dev/skb.c | 8 ++++++++
  1 file changed, 8 insertions(+)
drivers/net/can/dev/skb.c +0 -0
index 95fcdc1026f8..a0712e607b28 100644

drivers/net/can/dev/skb.c static void init_can_skb(struct sk_buff *skb) · line 217

Quoted 8 lines
drivers/net/can/dev/skb.c +5 -0
@@ -210,6 +210,14 @@ static void init_can_skb(struct sk_buff *skb)
  {
  	skb->pkt_type = PACKET_BROADCAST;
  	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	/* Restore the initialisations that were present before
+	 * 9f10374bb024 to prevent skb_under_panic in packet_rcv_spkt
+	 * and similar consumers.
+	 */

Please remove this comment in v2 - just add the code.

This is not usual and the patch description perfectly serves the documentation needs.

Thanks & best regards,
Oliver

Quoted 6 lines
drivers/net/can/dev/skb.c +3 -0
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
  }
 
  struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)