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

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

[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.

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")
Cc: stable@vger.kernel.org
Signed-off-by: zjamg <ndaugoing@gmail.com>
---
 drivers/net/can/dev/skb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..a0712e607b28 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -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)
-- 
2.53.0

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

[Oliver Hartkopp]

Hello zjamg!

Many thanks for the patch!

On 17.09.26 08:37, zjamg wrote:
> 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").

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.

> Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
> Cc: stable@vger.kernel.org
> Signed-off-by: zjamg <ndaugoing@gmail.com>

You can add my

Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

in your v2 patch.

> ---
>   drivers/net/can/dev/skb.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f8..a0712e607b28 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -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


> +	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)