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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.  packet_rcv() and tpacket_rcv() both wrap it in
> dev_has_header(), which is also the predicate the block comment at the
> top of the file states the restore in terms of; packet_rcv_spkt() does
> not.  commit d549699048b4 ("net/packet: fix packet receive on L3
> devices without visible hard header") introduced the helper and changed
> the two call sites, and this one stayed behind.
> 
> A device without a visible ll header can leave skb->mac_header at the
> 0xFFFF sentinel that __alloc_skb() initialises it to.  A CAN skb does:
> init_can_skb() sets pkt_type and ip_summed but does not reset the
> headers, and commit 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:ffffffff8bd21bc1 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:
>    <IRQ>
>    skb_push+0x38/0x40
>    packet_rcv_spkt+0xe1/0x170
>    __netif_receive_skb_core.constprop.0+0x7e8/0xd30
>    ...
>   Kernel panic - not syncing: Fatal exception in interrupt
> 
> The socket type is reachable: packet_create() accepts SOCK_PACKET
> alongside SOCK_RAW and SOCK_DGRAM behind the same CAP_NET_RAW check,
> and neither the socket length nor a capability check keeps it 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 did not initialise its
> mac header into a kernel panic.  Guard the push the way the other two
> receive paths do.
> 
> Tested on v7.3-rc5 under QEMU with a slcan device on a pty, which is
> the driver RX path: vcan does not reproduce it, because can_send()
> resets the headers on the way out.  One SOCK_PACKET socket bound to
> can0 and one frame written into the line discipline panics an
> unpatched kernel with the trace above; the same image with this patch
> prints no panic and powers off normally.  Both kernels are this tree,
> defconfig plus CONFIG_CAN_SLCAN=y, differing only in this hunk.
> 
> Fixes: d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header")
> Assisted-by: LLM
> Cc: stable@vger.kernel.org
> Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
> ---
> 
> Hello Oliver,
> 
> Removed the comment block, as you suggested.  You are right that the other
> two dev_has_header() call sites do not carry one, and that the block
> comment at the top of the file already states the invariant -- so the
> comment was only restating the commit message.  The hunk is the guard
> alone now, two lines.
> 
> No behaviour change from v2: same predicate, same push.  I re-ran the
> QEMU/slcan pair on this exact revision to be sure nothing else moved with
> it -- unpatched still panics with skb_under_panic len:-65455 out of
> packet_rcv_spkt+0xe1 in interrupt, patched prints no panic and powers off
> normally.  checkpatch is clean on the resulting diff.
> 
> Thanks for the review,
> Quchaosheng
>  net/packet/af_packet.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e01526ed..a951a1e3c088 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,8 @@ 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));
> +	if (dev_has_header(dev))
> +		skb_push(skb, skb->data - skb_mac_header(skb));

Since SOCK_PACKET is equivalent to SOCK_RAW, this looks like a
legitimate fix to me.

An earlier version of the patch gave a different reason: that CAN had
removed skb_reset_mac_header. This is a not a fix for that change,
agreed? Other locations in the stack also require it initialized, also
on devices without mac header.

I see that there is another CAN patch that reverts that change:
https://lore.kernel.org/linux-can/20260917123716.63116-1-ndaugoing@gmail.com/#r

Specifically to packet sockets, also see the detailed comments on this
point at the top of net/packet/af_packet.c

Small aside: please remember to not send more than 1 revision per 24h,
per Documentation/process/maintainer-netdev.rst

>  
>  	/*
>  	 *	The SOCK_PACKET socket receives _all_ frames.
> -- 
> 2.43.0
>