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

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

[Quchaosheng]

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));
 
 	/*
 	 *	The SOCK_PACKET socket receives _all_ frames.
-- 
2.43.0

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

[Willem de Bruijn]

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
> 

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

[Quchaosheng]

Agreed, this is not a fix for that change.  init_can_skb() is being fixed
separately -- zjamg's patch, now 05/22 in Marc's tree.

packet_rcv_spkt() is the only ll header restore that d549699048b4 left
without a dev_has_header() check, and SOCK_PACKET reaches it.  That is all
this patch is.

Nothing else queued for net.  I'll keep the 24 hours.

Thanks,
Quchaosheng

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

[Willem de Bruijn]

Quchaosheng wrote:
> Agreed, this is not a fix for that change.  init_can_skb() is being fixed
> separately -- zjamg's patch, now 05/22 in Marc's tree.
> 
> packet_rcv_spkt() is the only ll header restore that d549699048b4 left
> without a dev_has_header() check, and SOCK_PACKET reaches it.  That is all
> this patch is.

Perfect. Thanks for verifying.

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

[Willem de Bruijn]

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>

Reviewed-by: Willem de Bruijn <willemb@google.com>

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The history and Fixes attribution in the commit message are
  inaccurate.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same CAN
  RX skb with mac_header == 0xFFFF (left that way by init_can_skb()…
- [High] This problem wasn't introduced by this patch, but the
  SOCK_PACKET receive path this patch touches leaks stale skb->cb bytes…

--

net/packet: guard the ll header push in packet_rcv_spkt()

This patch wraps the link layer header restore in packet_rcv_spkt() with
dev_has_header(), as packet_rcv() and tpacket_rcv() already do. The
motivation is a skb_under_panic() on CAN RX skbs whose mac_header is left
at the 0xFFFF sentinel, because init_can_skb() no longer resets the headers.

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

[Severity: Medium]
Is this history accurate?

In net/packet/af_packet.c, d549699048b4 changed

    if (dev->header_ops)

to

    if (dev_has_header(dev))

in packet_rcv() and tpacket_rcv(), and it also updated the block comment at
the top of the file. packet_rcv_spkt() never had a header_ops guard. Its
unconditional skb_push() goes back to the initial git import
(1da177e4c3f4).

The panic in the commit message only happens when mac_header is left at
0xFFFF. That has been the case since 9f10374bb024 ("can: remove private
CAN skb headroom infrastructure") removed the skb_reset_*_header() calls
from init_can_skb_reserve() and init_can_skb(). The current init_can_skb()
in drivers/net/can/dev/skb.c only does:

    skb->pkt_type = PACKET_BROADCAST;
    skb->ip_summed = CHECKSUM_UNNECESSARY;

> Fixes: d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header")
> Cc: stable@vger.kernel.org

With this Fixes tag and Cc: stable, the patch would be queued for the 5.10+
stable trees as a panic fix. CAN RX skbs in those trees still reset their
headers, so this panic cannot happen there.

9f10374bb024 is the commit that makes the panic reachable, and it isn't
named in any tag. Should the Fixes tag point at 9f10374bb024 instead?

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

[Severity: High]
This isn't a bug introduced by this patch, but the same CAN RX skb with
mac_header == 0xFFFF also reaches generic XDP. This guard doesn't cover
that path.

netif_receive_generic_xdp() in net/core/dev.c does:

    mac_len = skb->data - skb_mac_header(skb);
    __skb_push(skb, mac_len);

Here mac_len comes out as roughly 4G - 65471, and __skb_push() only has a
DEBUG_NET warning. skb_headroom() then truncates to 65535, so no
reallocation happens, and the later __skb_pull() moves data back.

In bpf_prog_run_generic_xdp(), xdp_prepare_buff() gets a headroom of
skb_headroom - mac_len = 65535. That puts xdp->data at head + 65535, while
data_end is head + 80. The code then reads the Ethernet header at that
address:

    eth = (struct ethhdr *)xdp->data;
    orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);

If a generic XDP program is attached to the CAN interface, can this read
out of bounds for every received CAN frame?

On XDP_TX or XDP_REDIRECT, __skb_push(skb, mac_len) also moves skb->data
about 4 GiB below head and sets a wrapped skb->len. The skb is then
transmitted or redirected.

dev_xdp_attach() has no device type restriction for generic mode. When a
program is attached, do_xdp_generic() runs for every RX skb:

  slcan RX
    alloc_can_skb()->init_can_skb()   /* mac_header stays 0xFFFF */
    netif_rx()
      __netif_receive_skb_core()
        do_xdp_generic()
          netif_receive_generic_xdp()
            bpf_prog_run_generic_xdp()

The af_packet guard alone doesn't close the 9f10374bb024 regression. Does
the separate init_can_skb() fix mentioned in the commit message need to
land together with this one?

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

[Severity: High]
This is a pre-existing issue, but further down, packet_rcv_spkt() builds
the sockaddr_pkt in place in skb->cb:

    spkt->spkt_family = dev->type;
    strscpy(spkt->spkt_device, dev->name, sizeof(spkt->spkt_device));
    spkt->spkt_protocol = skb->protocol;

strscpy() writes strlen(name) + 1 bytes and doesn't pad, and nothing clears
the rest of spkt_device. packet_recvmsg() then copies the full
sizeof(struct sockaddr_pkt) to userspace:

    memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);

Can this leak stale skb->cb bytes to userspace?

On GRO RX paths, cb still holds struct napi_gro_cb, and dev_gro_receive()
stores a slab pointer and jiffies there:

    NAPI_GRO_CB(skb)->age = jiffies;
    NAPI_GRO_CB(skb)->last = skb;

Nothing clears these before ptype_all delivery, and skb_share_check() and
skb_clone() copy cb.

With a 1 or 2 character interface name, cb[4..7] holds the upper 32 bits
of a kernel sk_buff address. With a 4 to 12 character name, jiffies still
leaks.

packet_create() checks CAP_NET_RAW with ns_capable(). A user in their own
user and network namespace can therefore create a veth with GRO enabled
and pick its name.

This dates back to the strlcpy() version. 8fc9d51ea2d32 swapped in
strscpy(), which doesn't pad either.

Would strscpy_pad(), or a memset() of the sockaddr_pkt before filling it,
be appropriate here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928113108.2127215-1-quchaosheng000406%40163.com

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

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The history and Fixes attribution in the commit message are
>   inaccurate.
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the same CAN
>   RX skb with mac_header == 0xFFFF (left that way by init_can_skb()…
> - [High] This problem wasn't introduced by this patch, but the
>   SOCK_PACKET receive path this patch touches leaks stale skb->cb bytes…
> 

> [Severity: High]
> This is a pre-existing issue, but further down, packet_rcv_spkt() builds
> the sockaddr_pkt in place in skb->cb:
> 
>     spkt->spkt_family = dev->type;
>     strscpy(spkt->spkt_device, dev->name, sizeof(spkt->spkt_device));
>     spkt->spkt_protocol = skb->protocol;
> 
> strscpy() writes strlen(name) + 1 bytes and doesn't pad, and nothing clears
> the rest of spkt_device. packet_recvmsg() then copies the full
> sizeof(struct sockaddr_pkt) to userspace:
> 
>     memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);
> 
> Can this leak stale skb->cb bytes to userspace?

This is being addressed in a separate fix that is in the review process:

https://lore.kernel.org/netdev/20260919215237.3470987-1-benquike@gmail.com/