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

[PATCH 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 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")
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.

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

[Oliver Hartkopp]

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

[PATCH v2 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,

Yes -- af_packet.c is the right place, and your question made me cut the
patch back, so this is a v2.

You are right that the CAN side is already being fixed: I ran zjamg's v2
earlier today and verified it holds.  The two are not alternatives to each
other.  That patch restores the header initialisations the CAN stack lost,
which is the actual regression; this one is the packet socket that should
not panic when a link layer hands it an skb whose mac_header was never set.
Either one stops the crash on this path, but only the pair leaves the
producer correct and the receiver safe.

On your question about where it belongs: packet_rcv_spkt() is the third
call site of the same subtraction, and commit d549699048b4 changed the
other two to dev_has_header() and left this one alone.  It is still the
only one that does the subtraction unconditionally, and it is reachable
with SOCK_PACKET.  So this is that commit's missing hunk rather than a
second opinion on the CAN fix.

The v1 had an extra skb_mac_header_was_set(skb) conjunct and this version
drops it.  dev_has_header() alone is what the block comment at the top of
the file states the restore in terms of, and what packet_rcv() and
tpacket_rcv() test; for a device without a visible ll header the documented
invariant is that mac_header points at data, so the subtraction is a no-op
push and skipping it outright is the same thing.  CAN is the case where
that invariant does not hold, which is the panic; the extra conjunct only
would have masked it.  I have re-run both kernels with this version.

Details of the re-run, since the earlier numbers were from the v1: v7.3-rc5
under QEMU with slcan on a pty, defconfig plus CONFIG_CAN_SLCAN=y, two
images from one tree differing only in this hunk.  Unpatched: skb_under_panic
len:-65455, packet_rcv_spkt+0xe1, "Kernel panic - not syncing: Fatal
exception in interrupt".  With this patch: no panic, powers off normally.
checkpatch is clean apart from the unavoidable long line in the quoted trace.

Thanks,
Quchaosheng

 net/packet/af_packet.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 7c83e01526edc..40c67b86a1735 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1911,7 +1911,14 @@ 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));
+	/* Only a device with a visible ll header can have it restored, which is
+	 * the test packet_rcv() and tpacket_rcv() already make.  For the others
+	 * the header is invisible and the subtraction has no meaning: a producer
+	 * that left skb->mac_header at its 0xFFFF sentinel turns it into a huge
+	 * negative length that trips skb_under_panic() in softirq context.
+	 */
+	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 v2 net] net/packet: guard the ll header push in packet_rcv_spkt()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Quchaosheng]

Manual code inspection, and nothing reported it before this.

I was working out what else the CAN skb header regression (9f10374bb024)
reaches, and the packet socket turned out not to be covered by the CAN
patch: packet_rcv_spkt() is the one call site of the ll header restore
that d549699048b4 left without dev_has_header(), and SOCK_PACKET reaches
it.  It came out of reading the receive paths, not from a scan, a tool
or a report.

Because nothing had reported it, I built the reproduction before posting
rather than sending a theoretical path:

  v7.3-rc5 under QEMU, slcan on a pty (the driver RX path; vcan resets
  the headers in can_send() and does not reproduce it), defconfig plus
  CONFIG_CAN_SLCAN=y, two images from one tree differing only in the
  hunk.  Unpatched: skb_under_panic len:-65455 out of
  packet_rcv_spkt+0xe1 and a panic in interrupt.  Patched: no panic.

The v1 had an extra skb_mac_header_was_set() conjunct; v2 drops it after
Oliver asked whether af_packet.c was the right place for this, and both
kernels were re-run with the version posted here.

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

[Oliver Hartkopp]

On 28.09.26 10:09, 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,
> 
> Yes -- af_packet.c is the right place, and your question made me cut the
> patch back, so this is a v2.
> 

Thanks for the explanation.

> You are right that the CAN side is already being fixed: I ran zjamg's v2
> earlier today and verified it holds.  The two are not alternatives to each
> other.  That patch restores the header initialisations the CAN stack lost,
> which is the actual regression; this one is the packet socket that should
> not panic when a link layer hands it an skb whose mac_header was never set.
> Either one stops the crash on this path, but only the pair leaves the
> producer correct and the receiver safe.
> 
> On your question about where it belongs: packet_rcv_spkt() is the third
> call site of the same subtraction, and commit d549699048b4 changed the
> other two to dev_has_header() and left this one alone.  It is still the
> only one that does the subtraction unconditionally, and it is reachable
> with SOCK_PACKET.  So this is that commit's missing hunk rather than a
> second opinion on the CAN fix.

Correct!

> 
> The v1 had an extra skb_mac_header_was_set(skb) conjunct and this version
> drops it.  dev_has_header() alone is what the block comment at the top of
> the file states the restore in terms of, and what packet_rcv() and
> tpacket_rcv() test; for a device without a visible ll header the documented
> invariant is that mac_header points at data, so the subtraction is a no-op
> push and skipping it outright is the same thing.  CAN is the case where
> that invariant does not hold, which is the panic; the extra conjunct only
> would have masked it.  I have re-run both kernels with this version.
> 
> Details of the re-run, since the earlier numbers were from the v1: v7.3-rc5
> under QEMU with slcan on a pty, defconfig plus CONFIG_CAN_SLCAN=y, two
> images from one tree differing only in this hunk.  Unpatched: skb_under_panic
> len:-65455, packet_rcv_spkt+0xe1, "Kernel panic - not syncing: Fatal
> exception in interrupt".  With this patch: no panic, powers off normally.
> checkpatch is clean apart from the unavoidable long line in the quoted trace.
> 
> Thanks,
> Quchaosheng
> 
>   net/packet/af_packet.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e01526edc..40c67b86a1735 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,14 @@ 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));
> +	/* Only a device with a visible ll header can have it restored, which is
> +	 * the test packet_rcv() and tpacket_rcv() already make.  For the others
> +	 * the header is invisible and the subtraction has no meaning: a producer
> +	 * that left skb->mac_header at its 0xFFFF sentinel turns it into a huge
> +	 * negative length that trips skb_under_panic() in softirq context.
> +	 */

Just a nitpick: AI mostly likes to introduce comments that repeat the 
argumentation already provided in the commit message itself.

I would suggest to remove this entire comment as the other "if 
(dev_has_header(dev))" call sites don't have a comment either.

Comments are needed to provide additional information when the code is 
not obvious - and not the development history including producer 
failures ;-)

> +	if (dev_has_header(dev))
> +		skb_push(skb, skb->data - skb_mac_header(skb));
>   
>   	/*
>   	 *	The SOCK_PACKET socket receives _all_ frames.

Many thanks and best regards,
Oliver