Re: [PATCH net] net/packet: guard the ll header push in packet_rcv_spkt()

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 28.09.26 08:50, Quchaosheng wrote:
> 
> packet_rcv_spkt() restores the link layer header with
> 
> 	skb_push(skb, skb->data - skb_mac_header(skb));
> 
> That subtraction is only meaningful when the device actually has a link
> layer header and the producer initialised skb->mac_header.  A CAN skb
> does not: init_can_skb() sets pkt_type and ip_summed but leaves
> skb->mac_header at the 0xFFFF sentinel, because 9f10374bb024 ("can:
> remove private CAN skb headroom infrastructure") dropped the
> skb_reset_*_header() calls that used to be there.
> 
> skb_mac_header() is then 0xFFFF, the length becomes a large negative
> number and skb_push() reports it through skb_under_panic() -- from
> softirq context, so it is a full system panic even with panic_on_oops=0:
> 
>    skbuff: skb_under_panic: text:ffffffffadd28261 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
>    kernel BUG at net/core/skbuff.c:214!
>    RIP: 0010:skb_panic+0x50/0x60
>    Call Trace:
>     <TASK>
>     skb_push+0x4d/0x60
>     packet_rcv_spkt+0xe1/0x170
>     ...
>    Kernel panic - not syncing: Fatal exception in interrupt
> 
> packet_rcv() and tpacket_rcv() already handle this: both wrap their
> skb_push() in dev_has_header(dev).  packet_rcv_spkt() is the only
> remaining receive path that does the subtraction unconditionally, and it
> is reachable with SOCK_PACKET -- a socket type that still works and that
> no length or capability check keeps away from a CAN interface.
> 
> The missing skb_reset_*_header() calls in init_can_skb() are a regression
> in their own right and are being fixed separately, but a packet socket
> should not turn a link layer that forgot to initialise its mac header into
> a kernel panic.  Guard the push the same way the other two paths do.
> 
> Tested on v7.3.0-rc5 under QEMU with a slcan device on a pty (the driver
> RX path is required; vcan resets the headers on the way out and does not
> reproduce it).  A SOCK_PACKET socket on can0 panics an unpatched kernel
> with the trace above and is silent with this patch applied.
> 
> Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")

I wonder if we should fix this in af_packet.c ?

There's already a patch waiting for upstream in the CAN subsystem, where 
the problem has originally been introduced:

[PATCH v2] can: restore skb header initialisations in init_can_skb()
https://lore.kernel.org/linux-can/20260917123716.63116-1-ndaugoing@gmail.com/

Best regards,
Oliver

> Assisted-by: LLM
> Cc: stable@vger.kernel.org
> Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
> ---
>   net/packet/af_packet.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e0152..ab8e0309e 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,17 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>   
>   	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
>   
> -	skb_push(skb, skb->data - skb_mac_header(skb));
> +	/* The push restores the link layer header, which only exists for
> +	 * devices that have one.  A device without a visible ll header
> +	 * (ARPHRD_CAN and other L3 types) must not be pushed, and the
> +	 * subtraction is meaningless when the producer never initialised
> +	 * skb->mac_header -- it then holds the 0xFFFF sentinel and the
> +	 * result is a huge negative length that trips skb_under_panic() in
> +	 * softirq context.  packet_rcv() and tpacket_rcv() already guard
> +	 * this with dev_has_header(); do the same here.
> +	 */
> +	if (dev_has_header(dev) && skb_mac_header_was_set(skb))
> +		skb_push(skb, skb->data - skb_mac_header(skb));
>   
>   	/*
>   	 *	The SOCK_PACKET socket receives _all_ frames.
>